Commit Graph

58162 Commits

Author SHA1 Message Date
Heikki Linnakangas e2e3b8ae9e Disable tests using injection points in installcheck
The 'gin' test injections faults to GIN index build. If another test
running concurrently in the same cluster also tries to create a GIN
index, it will hit the fault, too.

To fix, disable tests using injection points when running against an
existing cluster. A better long-term solution would be to make the
injection points scoped to the database or process, but this will do
for now.

Discussion: https://www.postgresql.org/message-id/CA%2BhUKGJYhcG_o2nwSK6r01eOZJwNWUJUbX%3D%3DAVnW84f-%2B8yamQ@mail.gmail.com
Discussion: https://www.postgresql.org/message-id/10fd6cdd-c5d9-46fe-9fa1-7e661191309e@iki.fi
2024-03-15 13:06:57 +02:00
Michael Paquier 071e3ad59d Add basic TAP tests for the low-level backup method, take two
There are currently no tests for the low-level backup method where
pg_backup_start() and pg_backup_stop() are involved while taking a
file-system backup.  The tests introduced in this commit rely on a
background psql process to make sure that the backup is taken while the
session doing the SQL start and stop calls remains alive.

Two cases are checked here with the backup taken:
- Recovery without a backup_label, leading to a corrupted state.
- Recovery with a backup_label, with a consistent state reached.
Both cases cross-check some patterns in the logs generated when running
recovery.

Compared to the first attempt in 99b4a63bef, this includes a couple of
fixes making the CI stable (5 runs succeeded here):
- Add the file to the list of tests in meson.build.
- Race condition with the first WAL segment that we expect in the
primary's archives, by adding a poll on pg_stat_archiver.  The second
segment with the checkpoint record is archived thanks to pg_backup_stop
waiting for it.
- Fix failure of test where the backup_label does not exist.  The
cluster inherits the configuration of the first node; it was attempting
to store segments in the first node's archives, triggering failures with
copy on Windows.
- Fix failure of test on Windows because of incorrect parsing of the
backup_file in the success case.  The data of the backup_label file is
retrieved from the output pg_backup_stop() from a BackgroundPsql written
directly to the backup's data folder.  This would include CRLFs (\r\n),
causing the startup process to fail at the beginning of recovery when
parsing the backup_label because only LFs (\n) are allowed.

Author: David Steele
Discussion: https://postgr.es/m/f20fcc82-dadb-478d-beb4-1e2ffb0ace76@pgmasters.net
2024-03-15 08:29:54 +09:00
Michael Paquier cc5ef90edd Refactor initial hash lookup in dynahash.c
The same pattern is used three times in dynahash.c to retrieve a bucket
number and a hash bucket from a hash value.  This has popped up while
discussing improvements for the type cache, where this piece of
refactoring would become useful.

Note that hash_search_with_hash_value() does not need the bucket number,
just the hash bucket.

Author: Teodor Sigaev
Reviewed-by: Aleksander Alekseev, Michael Paquier
Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1@sigaev.ru
2024-03-15 07:57:17 +09:00
David Rowley 4169850f0b Trim ORDER BY/DISTINCT aggregate pathkeys in gather_grouping_paths
Similar to d8a295389, trim off any PathKeys which are for ORDER BY /
DISTINCT aggregate functions from the PathKey List for the Gather Merge
paths created by gather_grouping_paths().  These additional PathKeys are
not valid to use after grouping has taken place as these PathKeys belong
to columns which are inputs to an aggregate function and, therefore are
unavailable after aggregation.

Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/cf63174c-8c89-3953-cb49-48f41f74941a@gmail.com
Backpatch-through: 16, where 1349d2790 was added
2024-03-15 11:54:36 +13:00
Daniel Gustafsson 4665cebc8a Login event trigger documentation wordsmithing
Minor wordsmithing on the login trigger documentation and code
comments to improve readability, as well as fixing a few small
incorrect statements in the comments.

Author: Robert Treat <rob@xzilla.net>
Discussion: https://postgr.es/m/CAJSLCQ0aMWUh1m6E9YdjeqV61baQ=EhteJX8XOxXg8H_2Lcr0Q@mail.gmail.com
2024-03-14 23:35:35 +01:00
Tom Lane b4a71cf65d Make INSERT-from-multiple-VALUES-rows handle domain target columns.
Commit a3c7a993d fixed some cases involving target columns that are
arrays or composites by applying transformAssignedExpr to the VALUES
entries, and then stripping off any assignment ArrayRefs or
FieldStores that the transformation added.  But I forgot about domains
over arrays or composites :-(.  Such cases would either fail with
surprising complaints about mismatched datatypes, or insert unexpected
coercions that could lead to odd results.  To fix, extend the
stripping logic to get rid of CoerceToDomain if it's atop an ArrayRef
or FieldStore.

While poking at this, I realized that there's a poorly documented and
not-at-all-tested behavior nearby: we coerce each VALUES column to
the domain type separately, and rely on the rewriter to merge those
operations so that the domain constraints are checked only once.
If that merging did not happen, it's entirely possible that we'd get
unexpected domain constraint failures due to checking a
partially-updated container value.  There's no bug there, but while
we're here let's improve the commentary about it and add some test
cases that explicitly exercise that behavior.

Per bug #18393 from Pablo Kharo.  Back-patch to all supported
branches.

Discussion: https://postgr.es/m/18393-65fedb1a0de9260d@postgresql.org
2024-03-14 14:57:16 -04:00
Nathan Bossart d1162cfda8 Add pg_column_toast_chunk_id().
This function returns the chunk_id of an on-disk TOASTed value.  If
the value is un-TOASTed or not on-disk, it returns NULL.  This is
useful for identifying which values are actually TOASTed and for
investigating "unexpected chunk number" errors.

Bumps catversion.

Author: Yugo Nagata
Reviewed-by: Jian He
Discussion: https://postgr.es/m/20230329105507.d764497456eeac1ca491b5bd%40sraoss.co.jp
2024-03-14 10:58:00 -05:00
Heikki Linnakangas 84c18acaf6 Remove redundant snapshot copying from parallel leader to workers
The parallel query infrastructure copies the leader backend's active
snapshot to the worker processes. But BitmapHeapScan node also had
bespoken code to pass the snapshot from leader to the worker. That was
redundant, so remove it.

The removed code was analogous to the snapshot serialization in
table_parallelscan_initialize(), but that was the wrong role model. A
parallel bitmap heap scan is more like an independent non-parallel
bitmap heap scan in each parallel worker as far as the table AM is
concerned, because the coordination is done in nodeBitmapHeapscan.c,
and the table AM doesn't need to know anything about it.

This relies on the assumption that es_snapshot ==
GetActiveSnapshot(). That's not a new assumption, things would get
weird if you used the QueryDesc's snapshot for visibility checks in
the scans, but the active snapshot for evaluating quals, for
example. This could use some refactoring and cleanup, but for now,
just add some assertions.

Reviewed-by: Dilip Kumar, Robert Haas
Discussion: https://www.postgresql.org/message-id/5f3b9d59-0f43-419d-80ca-6d04c07cf61a@iki.fi
2024-03-14 15:18:10 +02:00
Robert Haas 2346df6fc3 Allow a no-wait lock acquisition to succeed in more cases.
We don't determine the position at which a process waiting for a lock
should insert itself into the wait queue until we reach ProcSleep(),
and we may at that point discover that we must insert ourselves ahead
of everyone who wants a conflicting lock, in which case we obtain the
lock immediately. Up until now, a no-wait lock acquisition would fail
in such cases, erroneously claiming that the lock couldn't be obtained
immediately.  Fix that by trying ProcSleep even in the no-wait case.

No back-patch for now, because I'm treating this as an improvement to
the existing no-wait feature. It could instead be argued that it's a
bug fix, on the theory that there should never be any case whatsoever
where no-wait fails to obtain a lock that would have been obtained
immediately without no-wait, but I'm reluctant to interpret the
semantics of no-wait that strictly.

Robert Haas and Jingxian Li

Discussion: http://postgr.es/m/CA+TgmobCH-kMXGVpb0BB-iNMdtcNkTvcZ4JBxDJows3kYM+GDg@mail.gmail.com
2024-03-14 08:56:06 -04:00
Alexander Korotkov c20d90a41c Fix contrib/pg_visibility/meson.build
I broke that in e85662df44 by oversight.
2024-03-14 13:38:44 +02:00
Alexander Korotkov eeefd4280f Add TAP tests for timeouts
This commit adds new tests to verify that transaction_timeout,
idle_session_timeout, and idle_in_transaction_session_timeout work as expected.
We introduce new injection points in before throwing a timeout FATAL error
and check these injection points are reached.

Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
Author: Andrey Borodin
Reviewed-by: Alexander Korotkov
2024-03-14 13:12:15 +02:00
Alexander Korotkov e85662df44 Fix false reports in pg_visibility
Currently, pg_visibility computes its xid horizon using the
GetOldestNonRemovableTransactionId().  The problem is that this horizon can
sometimes go backward.  That can lead to reporting false errors.

In order to fix that, this commit implements a new function
GetStrictOldestNonRemovableTransactionId().  This function computes the xid
horizon, which would be guaranteed to be newer or equal to any xid horizon
computed before.

We have to do the following to achieve this.

1. Ignore processes xmin's, because they consider connection to other databases
   that were ignored before.
2. Ignore KnownAssignedXids, because they are not database-aware. At the same
   time, the primary could compute its horizons database-aware.
3. Ignore walsender xmin, because it could go backward if some replication
   connections don't use replication slots.

As a result, we're using only currently running xids to compute the horizon.
Surely these would significantly sacrifice accuracy.  But we have to do so to
avoid reporting false errors.

Inspired by earlier patch by Daniel Shelepanov and the following discussion
with Robert Haas and Tom Lane.

Discussion: https://postgr.es/m/1649062270.289865713%40f403.i.mail.ru
Reviewed-by: Alexander Lakhin, Dmitry Koval
2024-03-14 13:12:05 +02:00
Alvaro Herrera cc6e64afda
Comment out noisy libpq_pipeline test
libpq_pipeline's new 'cancel' test needs more research; disable it
temporarily to prevent measles in the buildfarm.
2024-03-14 10:23:38 +01:00
Daniel Gustafsson 6b41ef0330 Fix documentation comment for pg_md5_hash
Commit b69aba7457 added the errstr parameter to pg_md5_hash but
missed updating the synopsis in the documentation comment.  The
follow-up commit 587de223f0 added the parameter to the list of
outputs.  The returnvalue had been changed from integer to bool
before that but remained in the synopsis.  This fixes both.

Author: Tatsuro Yamada <tatsuro.yamada@ntt.com>
Discussion: https://postgr.es/m/TYYPR01MB82313576150CC86084A122CD9E292@TYYPR01MB8231.jpnprd01.prod.outlook.com
2024-03-14 09:23:37 +01:00
Amit Kapila 9c40db3b02 Fix typos in reorderbuffer.c.
Author: Kyotaro Horiguchi
Discussion: https://postgr.es/m/20240314.132817.1496502692848380820.horikyota.ntt@gmail.com
2024-03-14 12:12:55 +05:30
Jeff Davis 2d819a08a1 Introduce "builtin" collation provider.
New provider for collations, like "libc" or "icu", but without any
external dependency.

Initially, the only locale supported by the builtin provider is "C",
which is identical to the libc provider's "C" locale. The libc
provider's "C" locale has always been treated as a special case that
uses an internal implementation, without using libc at all -- so the
new builtin provider uses the same implementation.

The builtin provider's locale is independent of the server environment
variables LC_COLLATE and LC_CTYPE. Using the builtin provider, the
database collation locale can be "C" while LC_COLLATE and LC_CTYPE are
set to "en_US", which is impossible with the libc provider.

By offering a new builtin provider, it clarifies that the semantics of
a collation using this provider will never depend on libc, and makes
it easier to document the behavior.

Discussion: https://postgr.es/m/ab925f69-5f9d-f85e-b87c-bd2a44798659@joeconway.com
Discussion: https://postgr.es/m/dd9261f4-7a98-4565-93ec-336c1c110d90@manitou-mail.org
Discussion: https://postgr.es/m/ff4c2f2f9c8fc7ca27c1c24ae37ecaeaeaff6b53.camel%40j-davis.com
Reviewed-by: Daniel Vérité, Peter Eisentraut, Jeremy Schneider
2024-03-13 23:33:44 -07:00
Peter Eisentraut 6ab2e8385d Put genbki.pl output into src/include/catalog/ directly
With the makefile rules, the output of genbki.pl was written to
src/backend/catalog/, and then the header files were linked to
src/include/catalog/.

This changes it so that the output files are written directly to
src/include/catalog/.  This makes the logic simpler, and it also makes
the behavior consistent with the meson build system.  Also, the list
of catalog files is now kept in parallel in
src/include/catalog/{meson.build,Makefile}, while before the makefiles
had it in src/backend/catalog/Makefile.

Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Discussion: https://www.postgresql.org/message-id/flat/21b74bdc-183d-4dd5-9c27-9378d178f459@eisentraut.org
2024-03-14 07:11:21 +01:00
Michael Paquier 6cb1b632b3 Revert "Add basic TAP tests for the low-level backup method"
This reverts commit 99b4a63bef.  The test is proving to be unstable,
so revert it for now.

One of the failures seen involves the cluster started without the
backup_label, where the archives of the primary are overwritten, causing
recovery failures on Windows.  This is simple to fix, but there is
another issue that's creeping behind in the form of an "invalid data in
file" ERROR when parsing the backup_label for the second recovery case,
as an effect of the backup_label data written after retrieving its
contents from pg_backup_stop().
_
Per buildfarm member sidewinder.
2024-03-14 13:19:12 +09:00
Michael Paquier 99b4a63bef Add basic TAP tests for the low-level backup method
There are currently no tests for the low-level backup method where
pg_backup_start() and pg_backup_stop() are involved while taking a
file-system backup.  The tests introduced in this commit rely on a
background psql process to make sure that the backup is taken while the
session doing the SQL start and stop calls remains alive.

Two cases are checked here with the backup taken:
- Recovery without a backup_label, leading to a corrupted state.
- Recovery with a backup_label, with a consistent state reached.
Both cases cross-check some patterns in the logs generated when running
recovery.

Author: David Steele
Discussion: https://postgr.es/m/f20fcc82-dadb-478d-beb4-1e2ffb0ace76@pgmasters.net
2024-03-14 10:49:52 +09:00
Alexander Korotkov e820db5b56 Improve documentation for pg_stat_checkpointer fields
pg_stat_checkpointer contains statistics for checkpoints and restartpoints.
Before 12915a58ee documentation said only about checkpoints implying that
restartpoint is the variation of checkpoint.  12915a58ee introduced
new separate statistics fields for restartpoints.  This commit explicitly
documents fields that are relevant for both checkpoints and restartpoints.

Reported-by: Magnus Hagander
Discussion: https://postgr.es/m/CABUevExav5-SR0x%2BG9kBUMV0G8XsvSUfuyyqmYBBJi6VHns6sw%40mail.gmail.com
Reviewed-by: Anton A. Melnikov
2024-03-14 02:17:59 +02:00
Nathan Bossart ecb0fd3372 Reintroduce MAINTAIN privilege and pg_maintain predefined role.
Roles with MAINTAIN on a relation may run VACUUM, ANALYZE, REINDEX,
REFRESH MATERIALIZE VIEW, CLUSTER, and LOCK TABLE on the relation.
Roles with privileges of pg_maintain may run those same commands on
all relations.

This was previously committed for v16, but it was reverted in
commit 151c22deee due to concerns about search_path tricks that
could be used to escalate privileges to the table owner.  Commits
2af07e2f74, 59825d1639, and c7ea3f4229 resolved these concerns by
restricting search_path when running maintenance commands.

Bumps catversion.

Reviewed-by: Jeff Davis
Discussion: https://postgr.es/m/20240305161235.GA3478007%40nathanxps13
2024-03-13 14:49:26 -05:00
Robert Haas 2041bc4276 Add the system identifier to backup manifests.
Before this patch, if you took a full backup on server A and then
tried to use the backup manifest to take an incremental backup on
server B, it wouldn't know that the manifest was from a different
server and so the incremental backup operation could potentially
complete without error. When you later tried to run pg_combinebackup,
you'd find out that your incremental backup was and always had been
invalid. That's poor timing, because nobody likes finding out about
backup problems only at restore time.

With this patch, you'll get an error when trying to take the (invalid)
incremental backup, which seems a lot nicer.

Amul Sul, revised by me. Review by Michael Paquier.

Discussion: http://postgr.es/m/CA+TgmoYLZzbSAMM3cAjV4Y+iCRZn-bR9H2+Mdz7NdaJFU1Zb5w@mail.gmail.com
2024-03-13 15:12:33 -04:00
Alvaro Herrera 1ee910ce43
Hopefully make libpq_pipeline's new cancel test more reliable
The newly introduced cancel test in libpq_pipeline was flaky. It's not
completely clear why, but one option is that the check for "active" was
actually seeing the active state for the previous query. This change
should address any such race condition by first waiting until the
connection is reported as idle.

Author: Jelte Fennema-Nio <me@jeltef.nl>
Discussion: https://postgr.es/m/CAGECzQRvmUK5-d68A+cm+fgmfht9Dv2uZ28-qq3QiaF6EAZqPQ@mail.gmail.com
2024-03-13 19:55:09 +01:00
Robert Haas dbfc447165 Expose new function get_controlfile_by_exact_path().
This works just like get_controlfile(), but expects the path to the
control file rather than the path to the data directory that contains
the control file. This makes more sense in cases where the caller
has already constructed the path to the control file itself.

Amul Sul and Robert Haas, reviewed by Michael Paquier
2024-03-13 12:06:44 -04:00
Peter Eisentraut 97d85be365 Make the order of the header file includes consistent
Similar to commit 7e735035f2.

Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/CAMbWs4-WhpCFMbXCjtJ%2BFzmjfPrp7Hw1pk4p%2BZpU95Kh3ofZ1A%40mail.gmail.com
2024-03-13 15:07:00 +01:00
Dean Rasheed 97d4262683 doc: Improve a couple of places in the MERGE docs.
In the synopsis, make the syntax for merge_update consistent with the
syntax for a plain UPDATE command. It was missing the optional "ROW"
keyword that can be used in a multi-column assignment, and the option
to assign from a multi-column subquery, both of which have been
supported by MERGE since it was introduced.

In the parameters section for the with_query parameter, mention that
WITH RECURSIVE isn't supported, since this is different from plain
INSERT, UPDATE, and DELETE commands. While at it, move that entry to
the top of the list, for consistency with the other pages.

Back-patch to v15, where MERGE was introduced.

Discussion: https://postgr.es/m/CAEZATCWoQyWkMFfu7JXXQr8dA6%3DgxjhYzgpuBP2oz0QoJTxGWw%40mail.gmail.com
2024-03-13 13:16:24 +00:00
Andres Freund a3da95deee meson: macos: Avoid warnings on Sonoma
Starting with the Sonoma toolchain macos' linker emits warnings when the same
library is linked to twice. That's ill considered, as the same library can be
used by multiple subsidiary libraries. Luckily there's a flag to suppress that
warning.

On Ventura meson's default of -Wl,-undefined,dynamic_lookup caused warnings,
which we suppressed with -Wl,-undefined,error. Unfortunately that causes a
warning on Sonoma, which is absurd, as it's documented linker default. To
avoid that warning, only add -Wl,-undefined,error if it does not trigger
warnings. Luckily dynamic_lookup doesn't trigger a warning on Sonoma anymore.

Discussion: https://postgr.es/m/20231201040515.p5bshhhtfru7d3da@awork3.anarazel.de
Backpatch: 16-, where the meson build was added
2024-03-13 01:40:53 -07:00
Peter Eisentraut 6612185883 Fix incorrect format placeholders 2024-03-13 06:40:32 +01:00
Michael Paquier a189ed49d6 Add tests for more row patterns with COPY FROM .. (ON_ERROR ignore)
While digging into the code of this feature, I got confused by the fact
that a line is skipped when a value cannot be converted to its expected
attribute even if the line has fewer attributes than the target
relation.  The tests had a check for the case of an empty line, this
commit a couple more patterns where a line is incomplete, but skipped
because of a conversion error.

Reviewed-by: Bharath Rupireddy
Discussion: https://postgr.es/m/Ze_7kZqexdt0BiyC@paquier.xyz
2024-03-13 14:19:21 +09:00
Amit Kapila 0b84f5c419 Fix a random failure in 038_save_logical_slots_shutdown.pl.
The test ensures that all the WAL on the publisher is sent to the
subscriber before shutdown by comparing the confirmed_flush_lsn of the
associated slot with the shutdown_checkpoint WAL location. But if the
shutdown_checkpoint location falls into a new page in the WAL then the
check won't work. So, ensure that the shutdown_checkpoint WAL record
doesn't fall into a new page.

Reported-by: Bharath Rupireddy
Author: Bharath Rupireddy
Reviewed-by: Vignesh C, Kuroda Hayato, Amit Kapila
Discussion: https://postgr.es/m/CALj2ACVLzH5CN-h9=S26mdRHPuJ9yDLUw70yh4JOiPw03WL0CQ@mail.gmail.com
2024-03-13 08:33:26 +05:30
Thomas Munro 0265e5c120 ci: Use a RAM disk and more CPUs on FreeBSD.
Run the tests in a RAM disk.  It's still a UFS file system and is backed
by 20GB of disk, but this avoids a lot of I/O.  Even though we disable
fsync, our tests do a lot of directory manipulations, some of which
force file system meta-data to disk and flush slow device write caches
on UFS.  This was a bottleneck preventing effective scaling beyond 2
CPUs.

Now we can use 4 CPUs like on other OSes, for a huge speedup.

Reviewed-by: Maxim Orlov <orlovmg@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKG%2BFXLcEg1dyTqJjDiNQ8pGom4KrJj4wF38C90thti9dVA%40mail.gmail.com
2024-03-13 14:58:27 +13:00
Michael Paquier 77cf6a78de Add some asserts based on LWLockHeldByMe() for replication slot statistics
Two assertions checking that ReplicationSlotAllocationLock is acquired
are added to pgstat_create_replslot() and pgstat_drop_replslot(),
corresponding to the routines in charge of the creation and the drop of
replication slot statistics.  The code previously relied on this
assumption and documented it in comments, but did not enforce this
policy at runtime.

Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/Ze_p-hmD_yFeVYXg@paquier.xyz
2024-03-13 07:45:11 +09:00
Jeff Davis 32dd2c1eff Fix version check in 002_pg_upgrade.pl.
Commit f696c0cd5f tried to account for the version in a way that
includes development versions, but it was broken. Fix with suggestion
from Tom Lane.

Discussion: https://postgr.es/m/1553991.1710191312@sss.pgh.pa.us
Reported-by: Tom Lane
2024-03-12 15:24:03 -07:00
Tom Lane 6ee3261e9b Fix confusion about the return rowtype of SQL-language procedures.
There is a very ancient hack in check_sql_fn_retval that allows a
single SELECT targetlist entry of composite type to be taken as
supplying all the output columns of a function returning composite.
(This is grotty and fundamentally ambiguous, but it's really hard
to do nested composite-returning functions without it.)

As far as I know, that doesn't cause any problems in ordinary
functions.  It's disastrous for procedures however.  All procedures
that have any output parameters are labeled with prorettype RECORD,
and the CALL code expects it will get back a record with one column
per output parameter, regardless of whether any of those parameters
is composite.  Doing something else leads to an assertion failure
or core dump.

This is simple enough to fix: we just need to not apply that rule
when considering procedures.  However, that requires adding another
argument to check_sql_fn_retval, which at least in principle might be
getting called by external callers.  Therefore, in the back branches
convert check_sql_fn_retval into an ABI-preserving wrapper around a
new function check_sql_fn_retval_ext.

Per report from Yahor Yuzefovich.  This has been broken since we
implemented procedures, so back-patch to all supported branches.

Discussion: https://postgr.es/m/CABz5gWHSjj2df6uG0NRiDhZ_Uz=Y8t0FJP-_SVSsRsnrQT76Gg@mail.gmail.com
2024-03-12 18:16:25 -04:00
David Rowley fe4750effd Fix incorrect filename reference in comment
Author: Cary Huang
Discussion: https://postgr.es/m/18e34071af0.dbfc9663424635.8571906799773344646@highgo.ca
2024-03-13 09:34:11 +13:00
Alvaro Herrera 61461a300c
libpq: Add encrypted and non-blocking query cancellation routines
The existing PQcancel API uses blocking IO, which makes PQcancel
impossible to use in an event loop based codebase without blocking the
event loop until the call returns.  It also doesn't encrypt the
connection over which the cancel request is sent, even when the original
connection required encryption.

This commit adds a PQcancelConn struct and assorted functions, which
provide a better mechanism of sending cancel requests; in particular all
the encryption used in the original connection are also used in the
cancel connection.  The main entry points are:

- PQcancelCreate creates the PQcancelConn based on the original
  connection (but does not establish an actual connection).
- PQcancelStart can be used to initiate non-blocking cancel requests,
  using encryption if the original connection did so, which must be
  pumped using
- PQcancelPoll.
- PQcancelReset puts a PQcancelConn back in state so that it can be
  reused to send a new cancel request to the same connection.
- PQcancelBlocking is a simpler-to-use blocking API that still uses
  encryption.

Additional functions are
 - PQcancelStatus, mimicks PQstatus;
 - PQcancelSocket, mimicks PQcancelSocket;
 - PQcancelErrorMessage, mimicks PQerrorMessage;
 - PQcancelFinish, mimicks PQfinish.

Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Reviewed-by: Denis Laxalde <denis.laxalde@dalibo.com>
Discussion: https://postgr.es/m/AM5PR83MB0178D3B31CA1B6EC4A8ECC42F7529@AM5PR83MB0178.EURPRD83.prod.outlook.com
2024-03-12 17:32:25 +01:00
Heikki Linnakangas cb9663e20d Fix copying SockAddr struct
Valgrind alerted about accessing uninitialized bytes after commit
4945e4ed4a:

==700242== VALGRINDERROR-BEGIN
==700242== Conditional jump or move depends on uninitialised value(s)
==700242==    at 0x6D8A2A: getnameinfo_unix (ip.c:253)
==700242==    by 0x6D8BD1: pg_getnameinfo_all (ip.c:122)
==700242==    by 0x4B3EB6: BackendInitialize (postmaster.c:4266)
==700242==    by 0x4B684E: BackendStartup (postmaster.c:4114)
==700242==    by 0x4B6986: ServerLoop (postmaster.c:1780)
==700242==    by 0x4B80CA: PostmasterMain (postmaster.c:1478)
==700242==    by 0x3F7424: main (main.c:197)
==700242==  Uninitialised value was created by a stack allocation
==700242==    at 0x4B6934: ServerLoop (postmaster.c:1737)
==700242==
==700242== VALGRINDERROR-END

That was because the SockAddr struct was not copied correctly.

Per buildfarm animal "skink".
2024-03-12 15:31:02 +02:00
Heikki Linnakangas 4945e4ed4a Move initialization of the Port struct to the child process
In postmaster, use a more lightweight ClientSocket struct that
encapsulates just the socket itself and the remote endpoint's address
that you get from accept() call. ClientSocket is passed to the child
process, which initializes the bigger Port struct. This makes it more
clear what information postmaster initializes, and what is left to the
child process.

Rename the StreamServerPort and StreamConnection functions to make it
more clear what they do. Remove StreamClose, replacing it with plain
closesocket() calls.

Reviewed-by: Tristan Partin, Andres Freund
Discussion: https://www.postgresql.org/message-id/7a59b073-5b5b-151e-7ed3-8b01ff7ce9ef@iki.fi
2024-03-12 13:42:38 +02:00
Heikki Linnakangas d162c3a73b Pass CAC as an argument to the backend process
We used to smuggle it to the child process in the Port struct, but it
seems better to pass it down as a separate argument. This paves the
way for the next commit, which moves the initialization of the Port
struct to the backend process, after forking.

Reviewed-by: Tristan Partin, Andres Freund
Discussion: https://www.postgresql.org/message-id/7a59b073-5b5b-151e-7ed3-8b01ff7ce9ef@iki.fi
2024-03-12 13:42:36 +02:00
Heikki Linnakangas 73f7fb2a4c Set socket options in child process after forking
Try to minimize the work done in the postmaster process for each
accepted connection, so that postmaster can quickly proceed with its
duties. These function calls are very fast so this doesn't make any
measurable performance difference in practice, but it's nice to have
all the socket options initialization code in one place for sake of
readability too. This also paves the way for an upcoming commit that
will move the initialization of the Port struct to the child process.

Discussion: https://www.postgresql.org/message-id/7a59b073-5b5b-151e-7ed3-8b01ff7ce9ef@iki.fi
2024-03-12 13:42:28 +02:00
Heikki Linnakangas f8c5317d00 Disconnect if socket cannot be put into non-blocking mode
Commit 387da18874 moved the code to put socket into non-blocking mode
from socket_set_nonblocking() into the one-time initialization
function, pq_init(). In socket_set_nonblocking(), there indeed was a
risk of recursion on failure like the comment said, but in pq_init(),
ERROR or FATAL is fine. There's even another elog(FATAL) just after
this, if setting FD_CLOEXEC fails.

Note that COMMERROR merely logged the error, it did not close the
connection, so if putting the socket to non-blocking mode failed we
would use the connection anyway. You might not immediately notice,
because most socket operations in a regular backend wait for the
socket to become readable/writable anyway. But e.g. replication will
be quite broken.

Backpatch to all supported versions.

Discussion: https://www.postgresql.org/message-id/d40a5cd0-2722-40c5-8755-12e9e811fa3c@iki.fi
2024-03-12 10:18:32 +02:00
Alvaro Herrera 4dec98c2af
libpq: Move pg_cancel to fe-cancel.c
No other files need to access this struct, so there is no need to have
its definition in a header file.

Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Discussion: https://postgr.es/m/202403061822.spfzqbf7dsgg@alvherre.pgsql
2024-03-12 09:11:24 +01:00
Michael Paquier d6e171fed6 Keep replication slot statistics on invalidation
The code path in charge of invalidating a replication slot includes a
call to pgstat_drop_replslot(), which would result in removing the
statistics of the slot once invalidated.  However, there is no need to
remove the statistics of an invalidated slot as one could still be
interested in looking at them to understand the activity of the slot
until its actual removal.

The initial design of the feature committed in be87200efd used the
approach to drop the slots, which is likely why the statistics were
still removed during the invalidation.

Another problem with this operation is that it was done without holding
ReplicationSlotAllocationLock, leaving it unprotected on concurrent
activity.  This part is arguably a bug, but that's a limited problem in
practice so no backpatch is done.

In passing, this commit adds a test to check this behavior.  The only
remaining code path where slot statistics are dropped now related to the
slot getting dropped.

Author: Bertrand Drouvot
Discussion: https://postgr.es/m/ZermH08Eq6YydHpO@ip-10-97-1-34.eu-west-3.compute.internal
2024-03-12 14:22:31 +09:00
Amit Kapila 397cd0b3c7 Remove redundant fetch of the recent flush pointer in WalSndWaitForWal.
In WalSndWaitForWal(), we fetch a recent flush pointer both outside the
loop and inside the loop. But we start using RecentFlushPtr only after we
fetch it inside the loop. So we can remove one outside the loop.

Author: Shveta Malik
Reviewed-by: Bertrand Drouvot, Matthias van de Meent, Amit Kapila
Discussion: https://postgr.es/m/CAJpy0uBSCQz1yMD-WiEthzEe23dti2-Kr_pitVb7vAPFbFKm=A@mail.gmail.com
2024-03-12 10:25:27 +05:30
Michael Paquier 2c8118ee5d Use printf's %m format instead of strerror(errno) in more places
Most callers of strerror() are removed from the backend code.  The
remaining callers require special handling with a saved errno from a
previous system call.  The frontend code still needs strerror() where
error states need to be handled outside of fprintf.

Note that pg_regress is not changed to use %m as the TAP output may
clobber errno, since those functions call fprintf() and friends before
evaluating the format string.

Support for %m in src/port/snprintf.c has been added in d6c55de1f9,
hence all the stable branches currently supported include it.

Author: Dagfinn Ilmari Mannsåker
Discussion: https://postgr.es/m/87sf13jhuw.fsf@wibble.ilmari.org
2024-03-12 10:02:54 +09:00
Peter Geoghegan 3045324214 Update obsolete index scan TID comments.
Oversight in commit c2fe139c20.
2024-03-11 18:07:10 -04:00
Jeff Davis bbbf71d9a6 Fix 002_pg_upgrade.pl.
Commit f696c0cd5f caused a test failure in 002_pg_upgrade.pl, because
an earlier s/// operator caused qr// to no longer match the empty
string. Use qr/^$/ instead, which is a better test anyway, because we
expect the stderr to be empty.

Initially this appeared to be a perl bug, but per discussion, it seems
that it was a misunderstanding of how perl works: an empty pattern
uses the last successful pattern. Given how surprising that behavior
is to perl non-experts, we will need to look for similar problems
elsewhere and eliminate the use of empty patterns throughout the
code. For now, address this one instance to fix the buildfarm.

Discussion: https://postgr.es/m/0ef325fa06e7a1605c4e119c4ecb637c67e5fb4e.camel@j-davis.com
Reviewed-by: Tom Lane
2024-03-11 14:09:07 -07:00
Alvaro Herrera 319e9e53f3
Add tests for libpq query cancellation APIs
This is in preparation of making changes and additions to these APIs.

Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Discussion: https://postgr.es/m/CAGECzQRb21spiiykQ48rzz8w+Hcykz+mB2_hxR65D9Qk6nnw=w@mail.gmail.com
2024-03-11 21:54:03 +01:00
Nathan Bossart 24c928ad9a reindexdb: Allow specifying objects to process in all databases.
Presently, reindexdb's --table, --schema, --index, and --system
options cannot be used together with --all, i.e., you cannot
specify objects to process in all databases.  This commit removes
this unnecessary restriction.  Furthermore, it removes the
restriction that --system cannot be used with --table, --schema,
and --index.  There is no such restriction for the latter options,
and there is no technical reason to disallow these combinations.

Reviewed-by: Kyotaro Horiguchi, Dean Rasheed
Discussion: https://postgr.es/m/20230628232402.GA1954626%40nathanxps13
2024-03-11 15:42:27 -05:00
Heikki Linnakangas 3d8652cd32 Remove unneeded vacuum_delay_point from heap_vac_scan_get_next_block
heap_vac_scan_get_next_block() does relatively little work, so there
is no need to call vacuum_delay_point(). A future commit will call
heap_vac_scan_get_next_block() from a callback, and we would like to
avoid calling vacuum_delay_point() in that callback.

Author: Melanie Plageman
Discussion: https://postgr.es/m/CAAKRu_Yf3gvXGcCnqqfoq0Q8LX8UM-e-qbm_B1LeZh60f8WhWA%40mail.gmail.com
2024-03-11 20:45:33 +02:00