ecpg's lexer and parser files aren't normally processed by
pgindent, and unsurprisingly there's a lot of code in there
that doesn't really match project style. I spent some time
running pgindent over the fragments of these files that are
C code, and this is the result. This is in the same spirit
as commit 30ed71e42, though apparently Peter used a different
method for that one, since it didn't find these problems.
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
We shouldn't ask the client to use a protocol version later than the
one that they requested. To avoid that, if the client requests a
version newer than the latest one we support, set FrontendProtocol
to the latest version we support, not the requested version. Then,
use that value when building the NegotiateProtocolVersion message.
(It seems good on general principle to avoid setting FrontendProtocol
to a version we don't support, anyway.)
None of this really matters right now, because we only support a
single protocol version, but if that ever changes, we'll need this.
Jelte Fennema-Nio, reviewed by me and incorporating some of my
proposed wording
Discussion: https://postgr.es/m/CAGECzQTyXDNtMXdq2L-Wp=OvOCPa07r6+U_MGb==h90MrfT+fQ@mail.gmail.com
Currently mul_var() uses the schoolbook multiplication algorithm,
which is O(n^2) in the number of NBASE digits. To improve performance
for large inputs, convert the inputs to base NBASE^2 before
multiplying, which effectively halves the number of digits in each
input, theoretically speeding up the computation by a factor of 4. In
practice, the actual speedup for large inputs varies between around 3
and 6 times, depending on the system and compiler used. In turn, this
significantly reduces the runtime of the numeric_big regression test.
For this to work, 64-bit integers are required for the products of
base-NBASE^2 digits, so this works best on 64-bit machines, on which
it is faster whenever the shorter input has more than 4 or 5 NBASE
digits. On 32-bit machines, the additional overheads, especially
during carry propagation and the final conversion back to base-NBASE,
are significantly higher, and it is only faster when the shorter input
has more than around 50 NBASE digits. When the shorter input has more
than 6 NBASE digits (so that mul_var_short() cannot be used), but
fewer than around 50 NBASE digits, there may be a noticeable slowdown
on 32-bit machines. That seems to be an acceptable tradeoff, given the
performance gains for other inputs, and the effort that would be
required to maintain code specifically targeting 32-bit machines.
Joel Jacobson and Dean Rasheed.
Discussion: https://postgr.es/m/9d8a4a42-c354-41f3-bbf3-199e1957db97%40app.fastmail.com
Commit ca481d3c9a introduced mul_var_short(), which is used by
mul_var() whenever the shorter input has 1-4 NBASE digits and the
exact product is requested. As speculated on in that commit, it can be
extended to work for more digits in the shorter input. This commit
extends it up to 6 NBASE digits (up to 24 decimal digits), for which
it also gives a significant speedup. This covers more cases likely to
occur in real-world queries, for which using base-NBASE^2 arithmetic
provides little benefit.
To avoid code bloat and duplication, refactor it a bit using macros
and exploiting the fact that some portions of the code are shared
between the different cases.
Dean Rasheed, reviewed by Joel Jacobson.
Discussion: https://postgr.es/m/9d8a4a42-c354-41f3-bbf3-199e1957db97%40app.fastmail.com
Attempting to add a system column for a table to an existing publication
would result in the not very intuitive error message of:
ERROR: negative bitmapset member not allowed
Here we improve that to have it display the same error message as a user
would see if they tried adding a system column for a table when adding
it to the publication in the first place.
Doing this requires making the function which validates the list of
columns an extern function. The signature of the static function wasn't
an ideal external API as it made the code more complex than it needed to be.
Here we adjust the function to have it populate a Bitmapset of attribute
numbers. Doing it this way allows code simplification.
There was no particular bug here other than the weird error message, so
no backpatch.
Bug: #18558
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Peter Smith, David Rowley
Discussion: https://postgr.es/m/18558-411bc81b03592125@postgresql.org
Since these are single bytes instead of v2 or v3 messages they need
custom tracing logic. These "messages" don't even have official names
in the protocol specification, so I (Jelte) called them SSLResponse and
GSSENCResponse here.
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Discussion: https://postgr.es/m/CAGECzQSoPHtZ4xe0raJ6FYSEiPPS+YWXBhOGo+Y1YecLgknF3g@mail.gmail.com
According to the commit message in 8ec569479, we must have all variables
in header files marked with PGDLLIMPORT. In commit d3cc5ffe81f6 some
variables were moved from launch_backend.c file to several header files.
This adds PGDLLIMPORT to moved variables.
Author: Sofia Kopikova <s.kopikova@postgrespro.ru>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/e0b17014-5319-4dd6-91cd-93d9c8fc9539%40postgrespro.ru
The TRACE_SORT macro guarded the availability of the trace_sort GUC
setting. But it has been enabled by default ever since it was
introduced in PostgreSQL 8.1, and there have been no reports that
someone wanted to disable it. So just remove the macro to simplify
things. (For the avoidance of doubt: The trace_sort GUC is still
there. This only removes the rarely-used macro guarding it.)
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://www.postgresql.org/message-id/flat/be5f7162-7c1d-44e3-9a78-74dcaa6529f2%40eisentraut.org
Historically, MinGW environments lacked some Windows API calls, so we
took a different code path in win32_langinfo(). Somehow, the code
change in commit 35eeea62 (removing setlocale() calls) caused one
particular 001_initdb.pl test to fail on MinGW + ICU builds, because
pg_import_system_collations() found no collations. It might take a
MinGW user to discover the exact reason.
Updating that function to use the same code as MSVC seems to fix that
test, so lets do that. (There are plenty more places that test for MSVC
unnecessarily, to be investigated later.)
While here, also rename the helper function win32_langinfo() to
win32_get_codeset(), to explain what it does less confusingly; it's not
really a general langinfo() substitute.
Noticed by triggering the optional MinGW CI task; no build farm animals
failed.
Discussion: https://postgr.es/m/CA%2BhUKGKBWfhXQ3J%2B2Lj5PhKvQnGD%3DsywA0XQcb7boTCf%3DerVLg%40mail.gmail.com
Previously, log_autovacuum_min_duration utilized dedicated code for
logging resource statistics, such as system and buffer usage during
autoanalyze. However, this logging functionality was not utilized by
ANALYZE VERBOSE.
This commit adds resource statistics reporting to ANALYZE VERBOSE by
reusing the same logging code as autoanalyze.
Author: Anthonin Bonnefoy
Reviewed-by: Masahiko Sawada
Discussion: https://postgr.es/m/CAO6_Xqr__kTTCLkftqS0qSCm-J7_xbRG3Ge2rWhucxQJMJhcRA%40mail.gmail.com
Previously, (auto)analyze used global variables VacuumPageHit,
VacuumPageMiss, and VacuumPageDirty to track buffer usage. However,
pgBufferUsage provides a more generic way to track buffer usage with
support functions.
This change replaces those global variables with pgBufferUsage in
analyze. Since analyze was the sole user of those variables, it
removes their declarations. Vacuum previously used those variables but
replaced them with pgBufferUsage as part of a bug fix, commit
5cd72cc0c.
Additionally, it adjusts the buffer usage message in both vacuum and
analyze for better consistency.
Author: Anthonin Bonnefoy
Reviewed-by: Masahiko Sawada, Michael Paquier
Discussion: https://postgr.es/m/CAO6_Xqr__kTTCLkftqS0qSCm-J7_xbRG3Ge2rWhucxQJMJhcRA%40mail.gmail.com
Commit 35eeea62 forgot to include <xlocale.h> when using locale_t
(which didn't seem to be required on newer Apple SDK as used by CI,
hence mistake). Let's see if this fixes build farm animals longfin and
sifika.
This gets rid of some setlocale() calls. The remaining call to
setlocale() in pg_get_encoding_from_locale() is a query of the name
of the current locale when none was provided (in a multi-threaded future
that would need more work).
All known non-Windows targets have nl_langinfo_l(), from POSIX 2008, and
for Windows we already do something thread-safe.
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/CA%2BhUKGJqVe0%2BPv9dvC9dSums_PXxGo9SWcxYAMBguWJUGbWz-A%40mail.gmail.com
We don't need configure probes for HAVE_LANGINFO_H (it is implied by
!WIN32), and we don't need to consider systems that have it but don't
define CODESET (that was for OpenBSD in commit 81cca218, but it has now
had it for 19 years).
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/CA%2BhUKGJqVe0%2BPv9dvC9dSums_PXxGo9SWcxYAMBguWJUGbWz-A%40mail.gmail.com
If tracing was enabled during connection startup, these messages would
previously be listed in the trace output as something like this:
F 54 Unknown message: 70
mismatched message length: consumed 4, expected 54
With this commit their type and contents are now correctly listed:
F 36 StartupMessage 3 0 "user" "foo" "database" "alvherre"
F 54 SASLInitialResponse "SCRAM-SHA-256" 32 'n,,n=,r=nq5zEPR/VREHEpOAZzH8Rujm'
F 108 SASLResponse 'c=biws,r=nq5zEPR/VREHEpOAZzH8RujmVtWZDQ8glcrvy9OMNw7ZqFUn,p=BBwAKe0WjSvigB6RsmmArAC+hwucLeuwJrR5C/HQD5M='
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAGECzQSoPHtZ4xe0raJ6FYSEiPPS+YWXBhOGo+Y1YecLgknF3g@mail.gmail.com
If a partition undergoes DETACH CONCURRENTLY immediately followed by
DROP, this could cause a problem for a concurrent transaction
recomputing the partition descriptor when running a prepared statement,
because it tries to dereference a pointer to a tuple that's not found in
a catalog scan.
The existing retry logic added in commit dbca3469ebf8 is sufficient to
cope with the overall problem, provided we don't try to dereference a
non-existant heap tuple.
Arguably, the code in RelationBuildPartitionDesc() has been wrong all
along, since no check was added in commit 898e5e3290a7 against receiving
a NULL tuple from the catalog scan; that bug has only become
user-visible with DETACH CONCURRENTLY which was added in branch 14.
Therefore, even though there's no known mechanism to cause a crash
because of this, backpatch the addition of such a check to all supported
branches. In branches prior to 14, this would cause the code to fail
with a "missing relpartbound for relation XYZ" error instead of
crashing; that's okay, because there are no reports of such behavior
anyway.
Author: Kuntal Ghosh <kuntalghosh.2007@gmail.com>
Reviewed-by: Junwang Zhao <zhjwpku@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/18559-b48286d2eacd9a4e@postgresql.org
Cluster.pm's wait_for_catchup and allied subroutines don't provide
enough information to diagnose the problem when a wait times out.
In hopes of debugging some intermittent buildfarm failures, let's
dump the ending state of the relevant system view when that happens.
Add this to v17 too, but not stable branches.
Discussion: https://postgr.es/m/352068.1723422725@sss.pgh.pa.us
We've had code for CRC-32 and CRC-32C for some time (for WAL
records, etc.), but there was no way for users to call it, despite
apparent popular demand. The new crc32() and crc32c() functions
accept bytea input and return bigint (to avoid returning negative
values).
Bumps catversion.
Author: Aleksander Alekseev
Reviewed-by: Peter Eisentraut, Tom Lane
Discussion: https://postgr.es/m/CAJ7c6TNMTGnqnG%3DyXXUQh9E88JDckmR45H2Q%2B%3DucaCLMOW1QQw%40mail.gmail.com
Much of the code in process_pm_child_exit() to launch replacement
processes when one exits or when progressing to next postmaster state
was unnecessary, because the ServerLoop will launch any missing
background processes anyway. Remove the redundant code and let
ServerLoop handle it.
In ServerLoop, move the code to launch all the processes to a new
subroutine, to group it all together.
Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://www.postgresql.org/message-id/8f2118b9-79e3-4af7-b2c9-bd5818193ca4@iki.fi
libpq must not use palloc/pfree. It's not allowed to exit on allocation
failure, and mixing the frontend pfree with malloc is architecturally
unsound.
Remove fe_memutils from the shlib build entirely, to keep devs from
accidentally depending on it in the future.
Author: Jacob Champion <jacob.champion@enterprisedb.com>
Discussion: https://www.postgresql.org/message-id/CAOYmi+=pg=W5L1h=3MEP_EB24jaBu2FyATrLXqQHGe7cpuvwyg@mail.gmail.com
The now preferred way to call realpath() is by passing NULL as the
second argument and get a malloc'ed result. We still supported the
old way of providing our own buffer as a second argument, for some
platforms that didn't support the new way yet. Those were only
Solaris less than version 11 and some older AIX versions (7.1 and
newer appear to support the new variant). We don't support those
platforms versions anymore, so we can remove this extra code.
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://www.postgresql.org/message-id/flat/9e638b49-5c3f-470f-a392-2cbedb2f7855%40eisentraut.org
32d3ed816 added the "path" column to pg_backend_memory_contexts to allow
a stable method of obtaining the parent MemoryContext of a given row in
the view. Using the "path" column is now the preferred method of
obtaining the parent row.
Previously, any queries which were self-joining to this view using the
"name" and "parent" columns could get incorrect results due to the fact
that names are not unique. Here we aim to explicitly break such queries
so that they can be corrected and use the "path" column instead.
It is possible that there are more innocent users of the parent column
that just need an indication of the parent and having to write out a
self-joining CTE may be an unnecessary hassle for those cases. Let's
remove the column for now and see if anyone comes back with any
complaints. This does seem like a good time to attempt to get rid of
the column as we still have around 1 year to revert this if someone comes
back with a valid complaint. Plus this view is new to v14 and is quite
niche, so perhaps not many people will be affected.
Author: Melih Mutlu <m.melihmutlu@gmail.com>
Discussion: https://postgr.es/m/CAGPVpCT7NOe4fZXRL8XaoxHpSXYTu6GTpULT_3E-HT9hzjoFRA@mail.gmail.com
Teach nbtree backwards scans to avoid relocking a just-read leaf page to
read its current left sibling link when it isn't truly necessary. This
happened inside _bt_readnextpage whenever _bt_readpage had already
determined that there'll be no further matches to the left (or at least
none for the current primitive index scan, for a scan with array keys).
A new precheck inside _bt_readnextpage is all that we need to avoid
these useless lock acquisitions. Arguably, using a precheck like this
was a missed opportunity for commit 2ed5b87f96, which taught nbtree to
drop leaf page pins early to avoid blocking cleanup by VACUUM. Forwards
scans already managed to avoid relocking the page like this.
The optimization added by this commit is particularly helpful with
backwards scans that use array keys where the scan must perform multiple
primitive index scans. Such backwards scans will now avoid a useless
leaf page re-lock at the end of each primitive index scan.
Note that this commit does not attempt to avoid needlessly re-locking a
leaf page that was just read when the scan must follow the leaf page's
left link. That more ambitious optimization could work by stashing the
left link when the page is first read by a backwards scan, allowing the
subsequent _bt_readnextpage call to optimistically skip re-reading the
original page just to get a new copy of its left link. For now we only
address cases where we don't care about our original page's left link.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Matthias van de Meent <boekewurm+postgres@gmail.com>
Discussion: https://postgr.es/m/CAH2-Wz=xgs7PojG=EUvhgadwENzu_mY_riNh-w9wFPsaS717ew@mail.gmail.com
Coverity complained that the hash_create() call might access
hash_table_ctl->hctl. That's a false alarm, hash_create() only
accesses that field when passed the HASH_SHARED_MEM flag. Try to
silence it by using a plain local variable instead of a const. That's
how the HASHCTL is initialized in all the other hash_create() calls.
Coverity thinks dpns->plan could be null at these points. That
shouldn't really be possible, but it's easy enough to modify the
Asserts so they'd not core-dump if it were true.
These are new in b919a97a6. Back-patch to v13; the v12 version
of the patch didn't have these Asserts.
The code intends to allow GUCs to be set within parallel workers
via function SET clauses, but not otherwise. However, doing so fails
for "session_authorization" and "role", because the assign hooks for
those attempt to set the subsidiary "is_superuser" GUC, and that call
falls foul of the "not otherwise" prohibition. We can't switch to
using GUC_ACTION_SAVE for this, so instead add a new GUC variable
flag GUC_ALLOW_IN_PARALLEL to mark is_superuser as being safe to set
anyway. (This is okay because is_superuser has context PGC_INTERNAL
and thus only hard-wired calls can change it. We'd need more thought
before applying the flag to other GUCs; but maybe there are other
use-cases.) This isn't the prettiest fix perhaps, but other
alternatives we thought of would be much more invasive.
While here, correct a thinko in commit 059de3ca4: when rejecting
a GUC setting within a parallel worker, we should return 0 not -1
if the ereport doesn't longjmp. (This seems to have no consequences
right now because no caller cares, but it's inconsistent.) Improve
the comments to try to forestall future confusion of the same kind.
Despite the lack of field complaints, this seems worth back-patching.
Thanks to Nathan Bossart for the idea to invent a new flag,
and for review.
Discussion: https://postgr.es/m/2833457.1723229039@sss.pgh.pa.us
pg_wal_replay_wait() is intended to be called on standby. However, standby
can be promoted to primary at any moment, even concurrently with the
pg_wal_replay_wait() call. If recovery is not currently in progress
that doesn't mean the wait was unsuccessful. Thus, we always need to recheck
if the target LSN is replayed.
Reported-by: Kevin Hale Boyes
Discussion: https://postgr.es/m/CAPpHfdu5QN%2BZGACS%2B7foxmr8_nekgA2PA%2B-G3BuOUrdBLBFb6Q%40mail.gmail.com
Author: Alexander Korotkov
Since the introduction of TID store, vacuum uses far less memory in
the common case than in versions 16 and earlier. Invoking multiple
rounds of index vacuuming in turn requires a much larger table. It'd
be a good idea anyway to cover this case in regression testing, and a
lower limit is less painful for slow buildfarm animals. The reason to
do it now is to re-enable coverage of the bugfix in commit 83c39a1f7f.
For consistency, give autovacuum_work_mem the same treatment.
Suggested by Andres Freund
Tested by Melanie Plageman
Backpatch to v17, where TID store was introduced
Discussion: https://postgr.es/m/20240516205458.ohvlzis5b5tvejru@awork3.anarazel.de
Discussion: https://postgr.es/m/20240722164745.fvaoh6g6zprisqgp%40awork3.anarazel.de
Some code using atol() would not work correctly if sizeof(long)==4:
- src/bin/pg_basebackup/pg_basebackup.c: Would miscount size of a
tablespace over 2 TB.
- src/bin/pg_basebackup/streamutil.c: Would truncate a timeline ID
beyond INT32_MAX.
- src/bin/pg_rewind/libpq_source.c: Would miscount size of files
larger than 2 GB (but this currently cannot happen).
Replace these with atoll().
In one case, the use of atol() did not result in incorrect behavior
but seems inconsistent with related code:
- src/interfaces/ecpg/ecpglib/execute.c: Gratuitous, since it
processes a value from pg_type.typlen, which is int16.
Replace this with atoi().
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://www.postgresql.org/message-id/flat/a52738ad-06bc-4d45-b59f-b38a8a89de49%40eisentraut.org
libpq tracing via PQtrace would uselessly print the wrong thing for
these types of messages. With this commit, their type and contents
would be correctly listed. (This can be verified with PQconnectStart(),
but we don't use that in libpq_pipeline, so I (Álvaro) haven't bothered
to add any tests.)
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Discussion: https://postgr.es/m/CAGECzQSoPHtZ4xe0raJ6FYSEiPPS+YWXBhOGo+Y1YecLgknF3g@mail.gmail.com
All child processes except the syslogger are killed on a restart. The
archiver might be already running though, if it was started during
recovery.
The split in the comments between "other special children" and the
first group of "background tasks" seemed really arbitrary, so I just
merged them all into one group.
Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://www.postgresql.org/message-id/8f2118b9-79e3-4af7-b2c9-bd5818193ca4@iki.fi
Currently, when a child process exits, the postmaster first scans
through BackgroundWorkerList, to see if it the child process was a
background worker. If not found, then it scans through BackendList to
see if it was a regular backend. That leads to some duplication
between the bgworker and regular backend cleanup code, as both have an
entry in the BackendList that needs to be cleaned up in the same way.
Refactor that so that we scan just the BackendList to find the child
process, and if it was a background worker, do the additional
bgworker-specific cleanup in addition to the normal Backend cleanup.
Change HandleChildCrash so that it doesn't try to handle the cleanup
of the process that already exited, only the signaling of all the
other processes. When called for any of the aux processes, the caller
had already cleared the *PID global variable, so the code in
HandleChildCrash() to do that was unused.
On Windows, if a child process exits with ERROR_WAIT_NO_CHILDREN, it's
now logged with that exit code, instead of 0. Also, if a bgworker
exits with ERROR_WAIT_NO_CHILDREN, it's now treated as crashed and is
restarted. Previously it was treated as a normal exit.
If a child process is not found in the BackendList, the log message
now calls it "untracked child process" rather than "server process".
Arguably that should be a PANIC, because we do track all the child
processes in the list, so failing to find a child process is highly
unexpected. But if we want to change that, let's discuss and do that
as a separate commit.
Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://www.postgresql.org/message-id/835232c0-a5f7-4f20-b95b-5b56ba57d741@iki.fi
This allows ForgetBackgroundWorker() and ReportBackgroundWorkerExit()
to take a RegisteredBgWorker pointer as argument, rather than a list
iterator. That feels a little more natural. But more importantly, this
paves the way for more refactoring in the next commit.
Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://www.postgresql.org/message-id/835232c0-a5f7-4f20-b95b-5b56ba57d741@iki.fi
Presently, we inconsistently use dashes in references to these
algorithms (e.g., CRC32C versus CRC-32C). Some popular web sources
appear to prefer dashes, and with this commit, we will, too.
Reviewed-by: Robert Haas
Discussion: https://postgr.es/m/ZrUFpLP-w2zTAHqq%40nathan
