Commit 9ce346eabf added startup
progress reporting, but begin_startup_progress_phase has a race
condition: the timeout for the previous phase might fire just
before we reschedule the interrupt for the next phase.
To avoid the race, disable the timeout, clear the flag, and then
re-enable the timeout.
Patch by me, reviewed by Nitin Jadhav.
Discussion: https://postgr.es/m/CA+TgmoYq38i6iAzfRLVxA6Cm+wMCf4WM8wC3o_a+X_JvWC8bJg@mail.gmail.com
The previous code used ThisTimeLineID, which need not even be
initialized here, although it usually was in practice, because
pg_basebackup issues IDENTIFY_SYSTEM before calling BASE_BACKUP,
and that initializes ThisTimeLineID as a side effect. That's not
really good enough, though, not only because we shoudn't be counting
on side effects like that, but also because the TLI could change
meanwhile. Fortunately, we have convenient access to more meaningful
TLI values, so use those instead.
Because of the way this logic is coded, the consequences of using
a possibly-incorrect TLI here are no worse than a slightly confusing
error message, I don't want to take any risk here, so no back-patch
at least for now.
Patch by me, reviewed by Kyotaro Horiguchi and Michael Paquier
Discussion: http://postgr.es/m/CA+TgmoZRNWGWYDX9RgTXMG6_nwSdB=PB-PPRUbvMUTGfmL2sHQ@mail.gmail.com
Commit d168b66682, which overhauled index deletion, added a
pg_unreachable() to the end of a sort comparator used when sorting heap
TIDs from an index page. This allows the compiler to apply
optimizations that assume that the heap TIDs from the index AM must
always be unique.
That doesn't seem like a good idea now, given recent reports of
corruption involving duplicate TIDs in indexes on Postgres 14. Demote
to an assertion, just in case.
Backpatch: 14-, where index deletion was overhauled.
We had a test showing that a variable isn't referenceable in its
own initialization expression, nor in prior ones in the same block.
It *is* referenceable in later expressions in the same block, but
AFAICS there is no test case exercising that. Add one, and also
add some error cases.
Also, document that this is possible, since the docs failed to
cover the point.
Per question from tomás at tuxteam. I don't feel any need to
back-patch this, but we should ensure we don't break it in future.
Discussion: https://postgr.es/m/20211029121435.GA5414@tuxteam.de
DST law changes in Fiji, Jordan, Palestine, and Samoa. Historical
corrections for Barbados, Cook Islands, Guyana, Niue, Portugal, and
Tonga.
Also, the Pacific/Enderbury zone has been renamed to Pacific/Kanton.
The following zones have been merged into nearby, more-populous zones
whose clocks have agreed since 1970: Africa/Accra, America/Atikokan,
America/Blanc-Sablon, America/Creston, America/Curacao,
America/Nassau, America/Port_of_Spain, Antarctica/DumontDUrville,
and Antarctica/Syowa.
This commit improves the speed of those tests by 25~30%, using some
simple ideas to reduce the amount of data written by pg_receivewal:
- Use a segment size of 1MB. While reducing the amount of data zeroed
by pg_receivewal for the new segments, this improves the code coverage
with a non-default segment size.
- In the last test involving a slot's restart_lsn, generate a checkpoint
to advance the redo LSN and the WAL retained by the slot created,
reducing the number of segments that need to be archived. This counts
for most of the gain.
- Minimize the amount of data inserted into the dummy table.
Reviewed-by: Ronan Dunklau
Discussion: https://postgr.es/m/YXqYKAdVEqmyTltK@paquier.xyz
Since the only possible divisors are 8, 10, and 16, it doesn't cost
much code space to replace the division loop with three copies using
constant divisors. On most machines, division by a constant can be
done a lot more cheaply than division by an arbitrary value.
A microbenchmark testing just snprintf("foo %d") with a 9-digit value
showed about a 2X speedup for me (tgl). Most of Postgres isn't too
dependent on the speed of snprintf, so that the effect in real-world
cases is barely measurable. Still, a cycle saved is a cycle earned.
Arjan van de Ven
Discussion: https://postgr.es/m/40a4b32a-b841-4667-11b2-a0baedb12714@linux.intel.com
Discussion: https://postgr.es/m/6e51c644-1b6d-956e-ac24-2d1b0541d532@linux.intel.com
Commits fdd965d07 and 3cd9c3b92 tested CREATE INDEX CONCURRENTLY by
launching two separate pgbench runs concurrently. This was needed so
that only a single client thread would run CREATE INDEX CONCURRENTLY,
avoiding deadlock between two CICs. However, there's a better way,
which is to use an advisory lock to prevent concurrent CICs. That's
better in part because the test code is shorter and more readable, but
mostly because it automatically scales things to launch an appropriate
number of CICs relative to the number of INSERT transactions.
As committed, typically half to three-quarters of the CIC transactions
were pointless because the INSERT transactions had already stopped.
In passing, remove background_pgbench, which was added to support
these tests and isn't needed anymore. We can always put it back
if we find a use for it later.
Back-patch to v12; older pgbench versions lack the
conditional-execution features needed for this method.
Tom Lane and Andrey Borodin
Discussion: https://postgr.es/m/139687.1635277318@sss.pgh.pa.us
This adds tests checking for the execution of both commands. The
recovery test 002_archiving.pl is nicely adapted to that, as promotion
is triggered already twice there, and even if any of those commands fail
they don't affect recovery or promotion.
A command success is checked using a file generated by an "echo"
command, that should be able to work in all the buildfarm environments,
even Msys (but we'll know soon about that). Command failure is tested
with an "echo" command that points to a path that does not exist,
scanning the backend logs to make sure that the failure happens. Both
rely on the backend triggering the commands from the root of the data
folder, making its logic more robust.
Thanks to Neha Sharma for the extra tests on Windows.
Author: Amul Sul, Michael Paquier
Reviewed-by: Andres Freund, Euler Taveira
Discussion: https://postgr.es/m/CAAJ_b95R_c4T5moq30qsybSU=eDzDHm=4SPiAWaiMWc2OW7=1Q@mail.gmail.com
Comments above _bt_findinsertloc() that talk about LP_DEAD items are now
out of place. We already discuss index tuple deletion at an earlier
point in the same comment block.
Oversight in commit d168b666.
Grant privileges on views pg_backend_memory_contexts and
pg_shmem_allocations to the role pg_read_all_stats. Also grant on the
underlying functions that those views depend on.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Nathan Bossart <bossartn@amazon.com>
Discussion: https://postgr.es/m/CALj2ACWAZo3Ar_EVsn2Zf9irG+hYK3cmh1KWhZS_Od45nd01RA@mail.gmail.com
Commit b4c4a00ea refactored the gist of the sslfiles target into a
separate makefile in order to override settings in Makefile.global.
The invocation of this this file didn't however include the absolute
path for VPATH builds, resulting in "make clean" failing. Fix by
providing the path to the new makefile.
Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20211026174152.jjcagswnbhxu7uqz@alap3.anarazel.de
As 98ec35b has proved, there has never been any coverage in this area of
the code. This commit adds a new TAP test with a template database that
includes a small set of shared dependencies copied to a new database.
The test is added in createdb, where we have never tested that -T
generates a query with TEMPLATE, either.
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/YXDTl+PfSnqmbbkE@paquier.xyz
A new option "FOR ALL TABLES IN SCHEMA" in Create/Alter Publication allows
one or more schemas to be specified, whose tables are selected by the
publisher for sending the data to the subscriber.
The new syntax allows specifying both the tables and schemas. For example:
CREATE PUBLICATION pub1 FOR TABLE t1,t2,t3, ALL TABLES IN SCHEMA s1,s2;
OR
ALTER PUBLICATION pub1 ADD TABLE t1,t2,t3, ALL TABLES IN SCHEMA s1,s2;
A new system table "pg_publication_namespace" has been added, to maintain
the schemas that the user wants to publish through the publication.
Modified the output plugin (pgoutput) to publish the changes if the
relation is part of schema publication.
Updates pg_dump to identify and dump schema publications. Updates the \d
family of commands to display schema publications and \dRp+ variant will
now display associated schemas if any.
Author: Vignesh C, Hou Zhijie, Amit Kapila
Syntax-Suggested-by: Tom Lane, Alvaro Herrera
Reviewed-by: Greg Nancarrow, Masahiko Sawada, Hou Zhijie, Amit Kapila, Haiying Tang, Ajin Cherian, Rahila Syed, Bharath Rupireddy, Mark Dilger
Tested-by: Haiying Tang
Discussion: https://www.postgresql.org/message-id/CALDaNm0OANxuJ6RXqwZsM1MSY4s19nuH3734j4a72etDwvBETQ@mail.gmail.com
Remove superuser check, allowing any user granted permissions on
pg_log_backend_memory_contexts() to log the memory contexts of any
backend.
Note that this could allow a privileged non-superuser to log the
memory contexts of a superuser backend, but as discussed, that does
not seem to be a problem.
Reviewed-by: Nathan Bossart, Bharath Rupireddy, Michael Paquier, Kyotaro Horiguchi, Andres Freund
Discussion: https://postgr.es/m/e5cf6684d17c8d1ef4904ae248605ccd6da03e72.camel@j-davis.com
The foreign data wrapper's validator function provides a HINT message with
list of valid options for the object specified in CREATE or ALTER command,
when the option given in the command is invalid. Previously
postgresql_fdw_validator() and the validator functions for postgres_fdw and
dblink_fdw worked in that way even there were no valid options in the object,
which could lead to the HINT message with empty list (because there were
no valid options). For example, ALTER FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (format 'csv') reported the following ERROR and HINT messages.
This behavior was confusing.
ERROR: invalid option "format"
HINT: Valid options in this context are:
There is no such issue in file_fdw. The validator function for file_fdw
reports the HINT message "There are no valid options in this context."
instead in that case.
This commit improves postgresql_fdw_validator() and the validator functions
for postgres_fdw and dblink_fdw so that they do likewise. For example,
this change causes the above ALTER FOREIGN DATA WRAPPER command to
report the following messages.
ERROR: invalid option "nonexistent"
HINT: There are no valid options in this context.
Author: Kosei Masumura
Reviewed-by: Bharath Rupireddy, Fujii Masao
Discussion: https://postgr.es/m/557d06cebe19081bfcc83ee2affc98d3@oss.nttdata.com
The previous coding relied on the memory for the slots being zeroed
elsewhere, which while it was true in this case is not an contract
which is guaranteed to hold. Explicitly clear the tts_isnull array
to ensure that the slots are filled from a known state.
Backpatch to v14 where the catalog multi-inserts were introduced.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAJ7c6TP0AowkUgNL6zcAK-s5HYsVHVBRWfu69FRubPpfwZGM9A@mail.gmail.com
Backpatch-through: 14
The case checking for a NULL output when a slot does not exist was
too lax, as it was passing for any output generated by the query. This
fixes the matching pattern to be what it should be, matching only on
"||".
Oversight in b4ada4e.
Prior to this patch, when running pg_receivewal, the streaming start
point would be the current location of the archives if anything is
found in the local directory where WAL segments are written, and
pg_receivewal would fall back to the current WAL flush location if there
are no archives, as of the result of an IDENTIFY_SYSTEM command.
If for some reason the WAL files from pg_receivewal were moved, it is
better to try a restart where we left at, which is the replication
slot's restart_lsn instead of skipping right to the current flush
location, to avoid holes in the WAL backed up. This commit changes
pg_receivewal to use the following sequence of methods to determine the
starting streaming LSN:
- Scan the local archives.
- Use the slot's restart_lsn, if supported by the backend and if a slot
is defined.
- Fallback to the current flush LSN as reported by IDENTIFY_SYSTEM.
To keep compatibility with older server versions, we only attempt to use
READ_REPLICATION_SLOT if the backend version is at least 15, and
fallback to the older behavior of streaming from the current flush
LSN if the command is not supported.
Some TAP tests are added to cover this feature.
Author: Ronan Dunklau
Reviewed-by: Kyotaro Horiguchi, Michael Paquier, Bharath Rupireddy
Discussion: https://postgr.es/m/18708360.4lzOvYHigE@aivenronan
Users sometimes get concerned whe they start the server and it
emits a few messages and then doesn't emit any more messages for
a long time. Generally, what's happening is either that the
system is taking a long time to apply WAL, or it's taking a
long time to reset unlogged relations, or it's taking a long
time to fsync the data directory, but it's not easy to tell
which is the case.
To fix that, add a new 'log_startup_progress_interval' setting,
by default 10s. When an operation that is known to be potentially
long-running takes more than this amount of time, we'll log a
status update each time this interval elapses.
To avoid undesirable log chatter, don't log anything about WAL
replay when in standby mode.
Nitin Jadhav and Robert Haas, reviewed by Amul Sul, Bharath
Rupireddy, Justin Pryzby, Michael Paquier, and Álvaro Herrera.
Discussion: https://postgr.es/m/CA+TgmoaHQrgDFOBwgY16XCoMtXxsrVGFB2jNCvb7-ubuEe1MGg@mail.gmail.com
Discussion: https://postgr.es/m/CAMm1aWaHF7VE69572_OLQ+MgpT5RUiUDgF1x5RrtkJBLdpRj3Q@mail.gmail.com
According to the comments, we initialize sendTimeLineIsHistoric
and sendTimeLine here for the benefit of WalSndSegmentOpen.
However, the only way that can happen is if logical_read_xlog_page
calls WALRead. And since logical_read_xlog_page initializes the
same global variables internally, we don't need to also do it here.
These initializations have been here since replication slots were
introduced in commit 858ec11858. They
were certainly useless at that time, too, because logical decoding
didn't yet exist then, and physical replication doesn't examine any
WAL at the time of slot creation. I haven't checked all the
intermediate versions, but I suspect there's no point at which
this code ever did anything useful.
To reduce future confusion, remove the code. Since there's no
functional defect, no back-patch.
Discussion: http://postgr.es/m/CA+TgmobSWzacEs+r6C-7DrOPDHoDar4i9gzxB3SCBr5qjnLmVQ@mail.gmail.com
All the code that runs in the startup process to write WAL records
before that's allowed generally is now consecutive, so there's no
reason to shut the facility to write WAL locally off and then turn
it on again three times in a row.
Unfortunately, this requires a slight kludge in the checkpointer,
which needs to separately enable writing WAL in order to write the
checkpoint record. Because that code might run in the same process
as StartupXLOG() if we are in single-user mode, we must save/restore
the state of the LocalXLogInsertAllowed flag. Hopefully, we'll be
able to eliminate this wart in further refactoring, but it's
not too bad anyway.
Amul Sul, with modifications by me.
Discussion: http://postgr.es/m/CAAJ_b97fysj6sRSQEfOHj-y8Jfd5uPqOgO74qast89B4WfD+TA@mail.gmail.com
This does a better job grouping related operations together, since
all of the WAL records that we need to write prior to allowing WAL
writes generally and written by a single uninterrupted stretch of code.
Since CleanupAfterArchiveRecovery() just (1) runs recovery_end_command,
(2) removes non-parent xlog files, and (3) archives any final partial
segment, this should be safe, because all of those things are pretty
much unrelated to the WAL record written by XLogReportParameters().
Amul Sul, per a suggestion from me
Discussion: http://postgr.es/m/CAAJ_b97fysj6sRSQEfOHj-y8Jfd5uPqOgO74qast89B4WfD+TA@mail.gmail.com
In selectnewtape(), use 'nOutputTapes' rather than 'nOutputRuns' in the
check for whether to start a new tape or to append a new run to an
existing tape. Until 'maxTapes' is reached, nOutputTapes is always equal
to nOutputRuns, so it doesn't change the logic, but it seems more logical
to compare # of tapes with # of tapes. Also, currently maxTapes is never
modified after the merging begins, but written this way, the code would
still work if it was. (Although the nOutputRuns == nOutputTapes assertion
would need to be removed and using nOutputRuns % nOutputTapes to
distribute the runs evenly across the tapes wouldn't do a good job
anymore).
Similarly in mergeruns(), change to USEMEM(state->tape_buffer_mem) to
account for the memory used for tape buffers. It's equal to availMem
currently, but tape_buffer_mem is more direct and future-proof. For
example, if we changed the logic to only allocate half of the remaining
memory to tape buffers, USEMEM(state->tape_buffer_mem) would still be
correct.
Coverity complained about these. Hopefully this patch helps it to
understand the logic better. Thanks to Tom Lane for initial analysis.
The command is supported for physical slots for now, and returns the
type of slot, its restart_lsn and its restart_tli.
This will be useful for an upcoming patch related to pg_receivewal, to
allow the tool to be able to stream from the position of a slot, rather
than the last WAL position flushed by the backend (as reported by
IDENTIFY_SYSTEM) if the archive directory is found as empty, which would
be an advantage in the case of switching to a different archive
locations with the same slot used to avoid holes in WAL segment
archives.
Author: Ronan Dunklau
Reviewed-by: Kyotaro Horiguchi, Michael Paquier, Bharath Rupireddy
Discussion: https://postgr.es/m/18708360.4lzOvYHigE@aivenronan
I found these by running pg_dump under "valgrind --leak-check=full".
The changes in flagInhIndexes() and getIndexes() replace allocation of
an array of which we use only some elements by individual allocations
of just the actually-needed objects. The previous coding wasted some
memory, but more importantly it confused valgrind's leak tracking.
collectComments() and collectSecLabels() remain major blots on
the valgrind report, because they don't PQclear their query
results, in order to avoid a lot of strdup's. That's a dubious
tradeoff, but I'll leave it alone here; an upcoming patch will
modify those functions enough to justify changing the tradeoff.
The five modules in our TAP test framework all had names in the top
level namespace. This is unwise because, even though we're not
exporting them to CPAN, the names can leak, for example if they are
exported by the RPM build process. We therefore move the modules to the
PostgreSQL::Test namespace. In the process PostgresNode is renamed to
Cluster, and TestLib is renamed to Utils. PostgresVersion becomes simply
PostgreSQL::Version, to avoid possible confusion about what it's the
version of.
Discussion: https://postgr.es/m/aede93a4-7d92-ef26-398f-5094944c2504@dunslane.net
Reviewed by Erik Rijkers and Michael Paquier
The purpose of commit 8a54e12a38 was to
fix this, and it sufficed when the PREPARE TRANSACTION completed before
the CIC looked for lock conflicts. Otherwise, things still broke. As
before, in a cluster having used CIC while having enabled prepared
transactions, queries that use the resulting index can silently fail to
find rows. It may be necessary to reindex to recover from past
occurrences; REINDEX CONCURRENTLY suffices. Fix this for future index
builds by making CIC wait for arbitrarily-recent prepared transactions
and for ordinary transactions that may yet PREPARE TRANSACTION. As part
of that, have PREPARE TRANSACTION transfer locks to its dummy PGPROC
before it calls ProcArrayClearTransaction(). Back-patch to 9.6 (all
supported versions).
Andrey Borodin, reviewed (in earlier versions) by Andres Freund.
Discussion: https://postgr.es/m/01824242-AA92-4FE9-9BA7-AEBAFFEA3D0C@yandex-team.ru
CIC and REINDEX CONCURRENTLY assume backends see their catalog changes
no later than each backend's next transaction start. That failed to
hold when a backend absorbed a relevant invalidation in the middle of
running RelationBuildDesc() on the CIC index. Queries that use the
resulting index can silently fail to find rows. Fix this for future
index builds by making RelationBuildDesc() loop until it finishes
without accepting a relevant invalidation. It may be necessary to
reindex to recover from past occurrences; REINDEX CONCURRENTLY suffices.
Back-patch to 9.6 (all supported versions).
Noah Misch and Andrey Borodin, reviewed (in earlier versions) by Andres
Freund.
Discussion: https://postgr.es/m/20210730022548.GA1940096@gust.leadboat.com
The documentation was imprecise about the starting LSN used for WAL
streaming if nothing can be found in the local archive directory
defined with the pg_receivewal command, so be more talkative on this
matter.
Extracted from a larger patch by the same author.
Author: Ronan Dunklau, Michael Paquier
Discussion: https://postgr.es/m/18708360.4lzOvYHigE@aivenronan
Backpatch-through: 10
Create a hash table that indexes dumpable objects by CatalogId
(that is, catalog OID + object OID). Use this to replace the
former catalogIdMap array, as well as various other single-
catalog index arrays, and also the extension membership map.
In principle this should be faster for databases with many objects,
since lookups are now O(1) not O(log N). However, it seems that these
lookups are pretty much negligible in context, so that no overall
performance change can be measured. But having only one lookup
data structure to maintain makes the code simpler and more flexible,
so let's do it anyway.
Discussion: https://postgr.es/m/2595220.1634855245@sss.pgh.pa.us
The code does not expect sh_error() to return, but the patch
that made this header usable in frontend didn't get that memo.
While here, plaster unlikely() on the tests that decide whether
to invoke sh_error(), and add our standard copyright notice.
Noted by Andres Freund. Back-patch to v13 where this frontend
support came in.
Discussion: https://postgr.es/m/0D54435C-1199-4361-9D74-2FBDCF8EA164@anarazel.de
Non-global default privilege entries should be dumped as-is,
not made relative to the default ACL for their object type.
This would typically only matter if one had revoked some
on-by-default privileges in a global entry, and then wanted
to grant them again in a non-global entry.
Per report from Boris Korzun. This is an old bug, so back-patch
to all supported branches.
Neil Chen, test case by Masahiko Sawada
Discussion: https://postgr.es/m/111621616618184@mail.yandex.ru
Discussion: https://postgr.es/m/CAA3qoJnr2+1dVJObNtfec=qW4Z0nz=A9+r5bZKoTSy5RDjskMw@mail.gmail.com
For non-MSVC builds this is make's $(CURDIR), while for MSVC builds it
is $topdir/$Config/$module. The directory is added as the second element
in the PATH, so that the install location takes precedence, but the
added PATH element takes precedence over the rest of the PATH.
The reason for this is to allow tests to find built products that are
not installed, such as the libpq_pipeline test driver.
The libpq_pipeline test is adjusted to take advantage of this.
Based on a suggestion from Andres Freund.
Backpatch to release 14.
Discussion: https://postgr.es/m/4941f5a5-2d50-1a0e-6701-14c5fefe92d6@dunslane.net
I just got burnt by trying to use pg_malloc instead of pg_malloc0
with this. Save the next hacker some time by not leaving this
API detail undocumented.
The certificate serial number generation was changed in b4c4a00ea to
use the current timestamp. The testharness must thus interrogate the
cert for the serialnumber using "openssl x509" which emits the serial
in hex format. Converting the serial to integer format to match whats
in pg_stat_ssl requires a 64-bit capable Perl. This adds a fallback
to checking for an integer when the tests with a 32-bit Perl.
Per failure on buildfarm member prairiedog.
Discussion: https://postgr.es/m/0D295F43-806D-4B3F-AB98-F941A19E0271@yesql.se
Using for a new database a template database with shared dependencies
that need to be copied over was causing a corruption of pg_shdepend
because of an off-by-one computation error of the index number used for
the values inserted with a slot.
Issue introduced by e3931d0. Monitoring the rest of the code, there are
no similar mistakes.
Reported-by: Sven Klemm
Author: Aleksander Alekseev
Reviewed-by: Daniel Gustafsson, Michael Paquier
Discussion: https://postgr.es/m/CAJ7c6TP0AowkUgNL6zcAK-s5HYsVHVBRWfu69FRubPpfwZGM9A@mail.gmail.com
Backpatch-through: 14
