Commit Graph

7151 Commits

Author SHA1 Message Date
David Rowley
fc4089f3c6 Fix possible crash in add_paths_to_append_rel()
While working on a8a968a82, I failed to consider that
cheapest_startup_path can be NULL when there is no non-parameterized
path in the pathlist.  This is well documented in set_cheapest(), I just
failed to notice.

Here we adjust the code to just check if the RelOptInfo has a
cheapest_startup_path set before adding it to the startup_subpaths list.

Reported-by: Richard Guo
Author: Richard Guo
Discussion: https://postgr.es/m/CAMbWs49w3t03V69XhdCuw+GDwivny4uQUxrkVp6Gejaspt0wMQ@mail.gmail.com
2023-10-10 16:50:03 +13:00
Michael Paquier
f483b20905 worker_spi: Fix another stability issue with BGWORKER_BYPASS_ALLOWCONN
worker_spi_launch() may report that a worker stopped when it fails to
connect on a database that does not allow connections if the worker
exits before the SQL function checks for the current status of the
worker.  The test is switched to use Cluster::psql instead of
safe_psql() so as it does not fail hard when this query errors.  While
on it, this removes a query that looks at pg_stat_activity to simplify
the test, as a check on the contents of the server logs achieves the
same when the worker cannot connect to the database without
datallowconn.

Per buildfarm members kestrel, mamba and serinus.  Bonus thanks to Tom
Lane for providing the logs of the failure from mamba that the buildfarm
was not able to show up.  Note that I have reproduced the failure with a
hardcoded stop point.

Discussion: https://postgr.es/m/3365937.1696801735@sss.pgh.pa.us
2023-10-10 09:04:28 +09:00
Amit Kapila
7cc2f59dd5 Remove duplicate words in docs and code comments.
Additionally, add a missing "the" in a couple of places.

Author: Vignesh C, Dagfinn Ilmari Mannsåker
Discussion: http://postgr.es/m/CALDaNm28t+wWyPfuyqEaARS810Je=dRFkaPertaLAEJYY2cWYQ@mail.gmail.com
2023-10-09 09:18:47 +05:30
Peter Eisentraut
ffb69b2311 Add test for checking the line length of --help output
There was some discussion what the line length should be.  Most output
currently clearly targets around 80 columns, but the maximum in use
currently is 95, so we set that as the current maximum.  This just
ensures that there is some guidance and there are no wild deviations.

based on patch by Atsushi Torikoshi <torikoshia@oss.nttdata.com>

Discussion: https://www.postgresql.org/message-id/flat/50ca8ff35a8dd8f9ec89963b503571a7@oss.nttdata.com
2023-10-06 11:56:19 +02:00
Michael Paquier
fd4d93d269 worker_spi: Fix test failure with BGWORKER_BYPASS_ALLOWCONN
A bgworker can spawn parallel workers of its own when executing queries,
and if the worker uses BGWORKER_BYPASS_ALLOWCONN while the database it
is connected to does not allow connections, a parallel worker would fail
to startup.  In the case of this module, the step checking for the
presence of the schema to create was spawning a worker, failing the last
test introduced by 991bb0f965.

This issue could be reproduced with debug_parallel_query = 'regress',
for example.

Per buildfarm member crake.
2023-10-06 09:56:55 +09:00
Michael Paquier
991bb0f965 worker_spi: Add tests for BGWORKER_BYPASS_ALLOWCONN
This bgworker flag exists in the core code since eed1ce72e1, but was
never tested.  This relies on 4f2994647f, that has added a way to
start dynamic workers with this flag enabled.

Reviewed-by: Bertrand Drouvot, Bharath Rupireddy
Discussion: https://postgr.es/m/bcc36259-7850-4882-97ef-d6b905d2fc51@gmail.com
2023-10-06 09:01:27 +09:00
David Rowley
a8a968a821 Consider cheap startup paths in add_paths_to_append_rel
6b94e7a6d did this for ordered append paths to allow fast startup
MergeAppends, however, nothing was done for the Append case.

Here we adjust add_paths_to_append_rel() to have it build an AppendPath
containing the cheapest startup paths from each of the child relations
when the append rel has "consider_startup" set.

Author: Andy Fan, David Rowley
Discussion: https://www.postgresql.org/message-id/CAKU4AWrXSkUV=Pt-gRxQT7EbfUeNssprGyNsB=5mJibFZ6S3ww@mail.gmail.com
2023-10-05 21:03:10 +13:00
Michael Paquier
4f2994647f worker_spi: Expand set of options to start workers
A couple of new options are added to this module to provide more control
on the ways bgworkers are started:
- A new GUC called worker_spi.role to control which role to use by
default when starting a worker.
- worker_spi_launch() gains three arguments: a role OID, a database OID
and flags (currently only BGWORKER_BYPASS_ALLOWCONN).  By default, the
role OID and the database OID are InvalidOid, in which case the worker
would use the related GUCs.

Workers loaded by shared_preload_libraries use the default values
provided by the GUCs, with flags at 0.  The options are given to the
main bgworker routine through bgw_extra.  A test case is tweaked to
start two dynamic workers with databases and roles defined by the caller
of worker_spi_launch().

These additions will have the advantage of expanding the tests for
bgworkers, for at least two cases:
- BGWORKER_BYPASS_ALLOWCONN has no coverage in the core tree.
- A new bgworker flag is under discussion, and this eases the
integration of new tests.

Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/bcc36259-7850-4882-97ef-d6b905d2fc51@gmail.com
2023-10-05 12:22:28 +09:00
Michael Paquier
3338a98382 test_shm_mq: Replace WAIT_EVENT_EXTENSION with custom wait events
Two custom wait events are added here:
- "TestShmMqBgWorkerStartup", when setting up a set of bgworkers in
wait_for_workers_to_become_ready().
- "TestShmMqMessageQueue", when waiting for a queued message in
test_shm_mq_pipelined().

Author: Masahiro Ikeda
Discussion: https://postgr.es/m/197bce267fa691a0ac62c86c4ab904c4@oss.nttdata.com
2023-10-04 17:12:25 +09:00
Michael Paquier
c8e318b1b8 worker_spi: Rename custom wait event to "WorkerSpiMain"
This naming is more consistent with all the other user-facing wait event
strings.  Other in-core modules will use the same naming convention, so
let's be consistent here as well.

Extracted from a larger patch by the same author.

Author: Masahiro Ikeda
Discussion: https://postgr.es/m/197bce267fa691a0ac62c86c4ab904c4@oss.nttdata.com
2023-10-04 16:20:41 +09:00
Peter Eisentraut
aea599cfc0 Fix incorrect format placeholder 2023-10-03 08:30:20 +02:00
David Rowley
2075ba9dc9 Tidy-up some appendStringInfo*() usages
Make a few newish calls to appendStringInfo() which have no special
formatting use appendStringInfoString() instead.  Also, adjust usages of
appendStringInfoString() which only append a string containing a single
character to make use of appendStringInfoChar() instead.

This makes the code marginally faster, but primarily this change is so
we use the StringInfo type as it was intended to be used.

Discussion: https://postgr.es/m/CAApHDvpXKQmL+r=VDNS98upqhr9yGBhv2Jw3GBFFk_wKHcB39A@mail.gmail.com
2023-10-03 17:09:52 +13:00
Michael Paquier
6c77bb42ab Replace use of stat()[7] by -s switch in TAP tests to retrieve file size
The list form of stat() is an inelegant API as it relies on the position
of the file size in the list returned in result.  Like in any other
places of the tree, replace that with a -s switch instead.

Another suggestion from Dagfinn is File::Stat, which we've been already
using for some other fields.  It really comes down to a matter of taste
to choose that over -s, and the latter is more used in the tree.

Author: Bertrand Drouvot
Reviewed-by: Dagfinn Ilmari Mannsåker
Discussion: https://postgr.es/m/b2020df7-d0fc-4ea5-b2a9-7efc6d36b2ac@gmail.com
2023-10-03 08:27:34 +09:00
Michael Paquier
2940f1c837 psql: Set variables from query result on failure when printing tuples
SetResultVariables() was not getting called when "printing" a result
that failed (see around PrintQueryResult), which would cause some
variables to not be set, like ROW_COUNT, SQLSTATE or ERROR.  This can be
confusing as a previous result would be retained.

This state could be reached when failing to process tuples in a few
commands, like \gset when it returns no tuples, or \crosstabview.  A
test is added, based on \gset.

This is arguably a bug fix, but no backpatch is done as there is a risk
of breaking scripts that rely on the previous behavior, even if they do
so accidentally.

Reported-by: amutu
Author: Japin Li
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/18134-87126d90cb4dd049@postgresql.org
2023-10-02 11:05:05 +09:00
Andrew Dunstan
276393f53e Only evaluate default values as required when doing COPY FROM
Commit 9f8377f7a2 was a little too eager in fetching default values.
Normally this would not matter, but if the default value is not valid
for the type (e.g. a varchar that's too long) it caused an unnecessary
error.

Complaint and fix from Laurenz Albe

Backpatch to release 16.

Discussion: https://postgr.es/m/75a7b7483aeb331aa017328d606d568fc715b90d.camel@cybertec.at
2023-10-01 10:18:41 -04:00
Andrew Dunstan
f6d4c9cf16 Provide FORCE_NULL * and FORCE_NOT_NULL * options for COPY FROM
These options already exist, but you need to specify a column list for
them, which can be cumbersome. We already have the possibility of all
columns for FORCE QUOTE, so this is simply extending that facility to
FORCE_NULL and FORCE_NOT_NULL.

Author: Zhang Mingli
Reviewed-By: Richard Guo, Kyatoro Horiguchi, Michael Paquier.

Discussion: https://postgr.es/m/CACJufxEnVqzOFtqhexF2+AwOKFrV8zHOY3y=p+gPK6eB14pn_w@mail.gmail.com
2023-09-30 12:34:41 -04:00
Dean Rasheed
1d5caec221 Fix EvalPlanQual rechecking during MERGE.
Under some circumstances, concurrent MERGE operations could lead to
inconsistent results, that varied according the plan chosen. This was
caused by a lack of rowmarks on the source relation, which meant that
EvalPlanQual rechecking was not guaranteed to return the same source
tuples when re-running the join query.

Fix by ensuring that preprocess_rowmarks() sets up PlanRowMarks for
all non-target relations used in MERGE, in the same way that it does
for UPDATE and DELETE.

Per bug #18103. Back-patch to v15, where MERGE was introduced.

Dean Rasheed, reviewed by Richard Guo.

Discussion: https://postgr.es/m/18103-c4386baab8e355e3%40postgresql.org
2023-09-30 10:52:21 +01:00
Thomas Munro
becfbdd6c1 Fix edge-case for xl_tot_len broken by bae868ca.
bae868ca removed a check that was still needed.  If you had an
xl_tot_len at the end of a page that was too small for a record header,
but not big enough to span onto the next page, we'd immediately perform
the CRC check using a bogus large length.  Because of arbitrary coding
differences between the CRC implementations on different platforms,
nothing very bad happened on common modern systems.  On systems using
the _sb8.c fallback we could segfault.

Restore that check, add a new assertion and supply a test for that case.
Back-patch to 12, like bae868ca.

Tested-by: Tom Lane <tgl@sss.pgh.pa.us>
Tested-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGLCkTT7zYjzOxuLGahBdQ%3DMcF%3Dz5ZvrjSOnW4EDhVjT-g%40mail.gmail.com
2023-09-26 10:53:38 +13:00
Nathan Bossart
13aeaf0797 Add worker type to pg_stat_subscription.
Thanks to commit 2a8b40e368, the logical replication worker type is
easily determined.  The worker type could already be deduced via
other columns such as leader_pid and relid, but that is unnecessary
complexity for users.

Bumps catversion.

Author: Peter Smith
Reviewed-by: Michael Paquier, Maxim Orlov, Amit Kapila
Discussion: https://postgr.es/m/CAHut%2BPtmbSMfErSk0S7xxVdZJ9XVE3xVLhqBTmT91kf57BeKDQ%40mail.gmail.com
2023-09-25 14:12:43 -07:00
Daniel Gustafsson
7750fefdb2 Add GUC for temporarily disabling event triggers
In order to troubleshoot misbehaving or buggy event triggers, the
documented advice is to enter single-user mode.  In an attempt to
reduce the number of situations where single-user mode is required
(or even recommended) for non-extraordinary maintenance, this GUC
allows to temporarily suspend event triggers.

This was originally extracted from a larger patchset which aimed
at supporting event triggers on login events.

Reviewed-by: Ted Yu <yuzhihong@gmail.com>
Reviewed-by: Mikhail Gribkov <youzhick@gmail.com>
Reviewed-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/9140106E-F9BF-4D85-8FC8-F2D3C094A6D9@yesql.se
Discussion: https://postgr.es/m/0d46d29f-4558-3af9-9c85-7774e14a7709@postgrespro.ru
2023-09-25 12:41:49 +02:00
Daniel Gustafsson
1f9e3a9be5 Fix typo in test comment
s/currect/correct/, accidentally introduced in 608b167f9f.
2023-09-23 09:56:38 +02:00
Thomas Munro
91b0e85aa0 Don't use Perl pack('Q') in 039_end_of_wal.pl.
'Q' for 64 bit integers turns out not to work on 32 bit Perl, as
revealed by the build farm.  Use 'II' instead, and deal with endianness.

Back-patch to 12, like bae868ca.

Discussion: https://postgr.es/m/ZQ4r1vHcryBsSi_V%40paquier.xyz
2023-09-23 14:13:06 +12:00
Thomas Munro
bae868caf2 Don't trust unvalidated xl_tot_len.
xl_tot_len comes first in a WAL record.  Usually we don't trust it to be
the true length until we've validated the record header.  If the record
header was split across two pages, previously we wouldn't do the
validation until after we'd already tried to allocate enough memory to
hold the record, which was bad because it might actually be garbage
bytes from a recycled WAL file, so we could try to allocate a lot of
memory.  Release 15 made it worse.

Since 70b4f82a4b, we'd at least generate an end-of-WAL condition if the
garbage 4 byte value happened to be > 1GB, but we'd still try to
allocate up to 1GB of memory bogusly otherwise.  That was an
improvement, but unfortunately release 15 tries to allocate another
object before that, so you could get a FATAL error and recovery could
fail.

We can fix both variants of the problem more fundamentally using
pre-existing page-level validation, if we just re-order some logic.

The new order of operations in the split-header case defers all memory
allocation based on xl_tot_len until we've read the following page.  At
that point we know that its first few bytes are not recycled data, by
checking its xlp_pageaddr, and that its xlp_rem_len agrees with
xl_tot_len on the preceding page.  That is strong evidence that
xl_tot_len was truly the start of a record that was logged.

This problem was most likely to occur on a standby, because
walreceiver.c recycles WAL files without zeroing out trailing regions of
each page.  We could fix that too, but it wouldn't protect us from rare
crash scenarios where the trailing zeroes don't make it to disk.

With reliable xl_tot_len validation in place, the ancient policy of
considering malloc failure to indicate corruption at end-of-WAL seems
quite surprising, but changing that is left for later work.

Also included is a new TAP test to exercise various cases of end-of-WAL
detection by writing contrived data into the WAL from Perl.

Back-patch to 12.  We decided not to put this change into the final
release of 11.

Author: Thomas Munro <thomas.munro@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Noah Misch <noah@leadboat.com> (the idea, not the code)
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Sergei Kornilov <sk@zsrv.org>
Reviewed-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/17928-aa92416a70ff44a2%40postgresql.org
2023-09-23 10:26:24 +12:00
Daniel Gustafsson
33774978c7 Avoid using internal test methods in SSL tests
The SSL tests for pg_ctl restart with an incorrect key passphrase used
the internal _update_pid method to set the pidfile after running pg_ctl
manually instead of using the supplied ->restart method. This refactors
the ->restart method to accept a fail_ok parameter like how ->start and
->stop does, and changes the SSL tests to use this instead. This removes
the need to call internal test module functions.

Reviewed-by: Melih Mutlu <m.melihmutlu@gmail.com>
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/F81643C4-D7B8-4C6B-AF18-B73839966279@yesql.se
2023-09-22 13:35:37 +02:00
Tom Lane
48e2b234f8 Fix COMMIT/ROLLBACK AND CHAIN in the presence of subtransactions.
In older branches, COMMIT/ROLLBACK AND CHAIN failed to propagate
the current transaction's properties to the new transaction if
there was any open subtransaction (unreleased savepoint).
Instead, some previous transaction's properties would be restored.
This is because the "if (s->chain)" check in CommitTransactionCommand
examined the wrong instance of the "chain" flag and falsely
concluded that it didn't need to save transaction properties.

Our regression tests would have noticed this, except they used
identical transaction properties for multiple tests in a row,
so that the faulty behavior was not distinguishable from correct
behavior.

Commit 12d768e70 fixed the problem in v15 and later, but only rather
accidentally, because I removed the "if (s->chain)" test to avoid a
compiler warning, while not realizing that the warning was flagging a
real bug.

In v14 and before, remove the if-test and save transaction properties
unconditionally; just as in the newer branches, that's not expensive
enough to justify thinking harder.

Add the comment and extra regression test to v15 and later to
forestall any future recurrence, but there's no live bug in those
branches.

Patch by me, per bug #18118 from Liu Xiang.  Back-patch to v12 where
the AND CHAIN feature was added.

Discussion: https://postgr.es/m/18118-4b72fcbb903aace6@postgresql.org
2023-09-21 23:11:30 -04:00
Michael Paquier
78a33bba4c Improve error message for snapshot import in snapmgr.c, take two
When a snapshot file fails to be read in ImportSnapshot(), it would
issue an ERROR as "invalid snapshot identifier" when opening a stream
for it in read-only mode.  The error handling is improved to be more
talkative in failure cases:
- If a snapshot identifier uses incorrect characters, complain with the
same error as before this commit.
- If the snapshot file cannot be found in pg_snapshots/, complain with a
"snapshot \"foo\" does not exist" instead.  This maps to the case where
AllocateFile() fails on ENOENT.  Based on a suggestion from Andres
Freund.
- If AllocateFile() throws something else than ENOENT as errno, report
it with more details in %m instead, as these failures are never
expected.

b29504eeb489 was the first improvement take.  The older error message
exists since bb446b689b that introduced snapshot imports.  Two test
cases are added to cover the cases of an identifier with an incorrect
format and of a missing snapshot.

Author: Bharath Rupireddy
Reviewed-by: Andres Freund, Daniel Gustafsson, Michael Paquier
Discussion: https://postgr.es/m/CALj2ACWmr=3KdxDkm8h7Zn1XxBoF6hdzq8WQyMn2y1OL5RYFrg@mail.gmail.com
2023-09-19 10:19:50 +09:00
Tom Lane
e0e492e5a9 Track nesting depth correctly when drilling down into RECORD Vars.
expandRecordVariable() failed to adjust the parse nesting structure
correctly when recursing to inspect an outer-level Var.  This could
result in assertion failures or core dumps in corner cases.

Likewise, get_name_for_var_field() failed to adjust the deparse
namespace stack correctly when recursing to inspect an outer-level
Var.  In this case the likely result was a "bogus varno" error
while deparsing a view.

Per bug #18077 from Jingzhou Fu.  Back-patch to all supported
branches.

Richard Guo, with some adjustments by me

Discussion: https://postgr.es/m/18077-b9db97c6e0ab45d8@postgresql.org
2023-09-15 17:01:52 -04:00
Amit Kapila
e0b2eed047 Flush logical slots to disk during a shutdown checkpoint if required.
It's entirely possible for a logical slot to have a confirmed_flush LSN
higher than the last value saved on disk while not being marked as dirty.
Currently, it is not a major problem but a later patch adding support for
the upgrade of slots relies on that value being properly flushed to disk.

It can also help avoid processing the same transactions again in some
boundary cases after the clean shutdown and restart.  Say, we process
some transactions for which we didn't send anything downstream (the
changes got filtered) but the confirm_flush LSN is updated due to
keepalives.  As we don't flush the latest value of confirm_flush LSN, it
may lead to processing the same changes again without this patch.

The approach taken by this patch has been suggested by Ashutosh Bapat.

Author: Vignesh C, Julien Rouhaud, Kuroda Hayato
Reviewed-by: Amit Kapila, Dilip Kumar, Michael Paquier, Ashutosh Bapat, Peter Smith, Hou Zhijie
Discussion: http://postgr.es/m/CAA4eK1JzJagMmb_E8D4au=GYQkxox0AfNBm1FbP7sy7t4YWXPQ@mail.gmail.com
Discussion: http://postgr.es/m/TYAPR01MB58664C81887B3AF2EB6B16E3F5939@TYAPR01MB5866.jpnprd01.prod.outlook.com
2023-09-14 08:57:05 +05:30
Amit Kapila
a2e0d5e5f6 Remove redundant result assignment in 004_sync.pl.
Author: Peter Smith
Discussion: http://postgr.es/m/CAHut+PuTNdxnpn24s6jfPDe+fKJoe3M-CoNv-DFsZmJN-ed0Xw@mail.gmail.com
2023-09-14 08:39:03 +05:30
Amit Kapila
f062cddafe Fix the ALTER SUBSCRIPTION to reflect the change in run_as_owner option.
Reported-by: Jeff Davis
Author: Hou Zhijie
Reviewed-by: Amit Kapila
Backpatch-through: 16
Discussion: http://postgr.es/m/17b62714fd115bd1899afd922954540a5c6a0467.camel@j-davis.com
2023-09-13 09:34:30 +05:30
Masahiko Sawada
28ed5ecbe0 Stabilize subscription stats test.
The new test added by commit 68a59f9e9 disables the subscription and
manually drops the associated replication slot. However, since
disabling the subsubscription doesn't wait for a walsender to release
the replication slot and exit, pg_drop_replication_slot() could
fail. Avoid failure by adding a wait for the replication slot to
become inactive.

Reported-by: Hou Zhijie, as per buildfarm
Reviewed-by: Hou Zhijie
Discussion: https://postgr.es/m/OS0PR01MB571682316378379AA34854F694E9A%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Backpatch-through: 15
2023-09-08 22:50:59 +09:00
Alvaro Herrera
ac22a9545c
Move privilege check to the right place
Now that ATExecDropConstraint doesn't recurse anymore, so it's wrong to
test privileges "during recursion" there.  Move the check to
dropconstraint_internal, which is the place where recursion occurs.

In passing, remove now-useless 'recursing' argument to
ATExecDropConstraint.

Discussion: https://postgr.es/m/202309051744.y4mndw5gwzhh@alvherre.pgsql
2023-09-07 12:15:18 +02:00
Thomas Munro
0174c2d213 Fix instability in 031_recovery_conflict.pl.
Where the test wants a VACUUM command to generate WAL that would
conflict with a session on the standby, it could transiently fail to do
so if it couldn't acquire a cleanup lock conditionally at that moment on
the primary.  VACUUM FREEZE will wait, so use that instead.

No back-patch for now, but that will be needed if/when the test is
re-enabled in back-branches.

Suggested-by: Andres Freund <andres@anarazel.de>
Reported-by: Christoph Berg <myon@debian.org>
Discussion: https://postgr.es/m/20230812210006.ei7tutzwcr5svyt6%40awork3.anarazel.de
2023-09-07 14:38:15 +12:00
Daniel Gustafsson
aca17fe206 Update comments to match location of definition
Commit cc50080a82 rearranged testsuites to reduce dependencies, but
missed to update a comment when moving an operator class definition.
Also fix a typo in that same comment while here.

Author: Suraj Kharage <suraj.kharage@enterprisedb.com>
Discussion: https://postgr.es/m/CAF1DzPWXd2yq9_=P905cEypMVKw3ho+Fpj4HwJ4ta8T-eh+Yig@mail.gmail.com
2023-09-06 10:18:30 +02:00
Thomas Munro
f691f5b80a Remove the "snapshot too old" feature.
Remove the old_snapshot_threshold setting and mechanism for producing
the error "snapshot too old", originally added by commit 848ef42b.
Unfortunately it had a number of known problems in terms of correctness
and performance, mostly reported by Andres in the course of his work on
snapshot scalability.  We agreed to remove it, after a long period
without an active plan to fix it.

This is certainly a desirable feature, and someone might propose a new
or improved implementation in the future.

Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CACG%3DezYV%2BEvO135fLRdVn-ZusfVsTY6cH1OZqWtezuEYH6ciQA%40mail.gmail.com
Discussion: https://postgr.es/m/20200401064008.qob7bfnnbu4w5cw4%40alap3.anarazel.de
Discussion: https://postgr.es/m/CA%2BTgmoY%3Daqf0zjTD%2B3dUWYkgMiNDegDLFjo%2B6ze%3DWtpik%2B3XqA%40mail.gmail.com
2023-09-05 19:53:43 +12:00
Alvaro Herrera
d0ec2ddbe0
Fix not-null constraint test
When a partitioned table has a primary key, trying to find the
corresponding not-null constraint for that column would come up empty,
causing code that's trying to check said not-null constraint to crash.
Fix by only running the check when the not-null constraint exists.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/d57b4a69-7394-3146-5976-9a1ef27e7972@gmail.com
2023-09-01 19:49:20 +02:00
Alvaro Herrera
e09d763e25
ATPrepAddPrimaryKey: ignore non-PK constraints
Because of lack of test coverage, this function added by b0e96f3119
wasn't ignoring constraint types other than primary keys, which it
should have.  Add some lines to a test for it.

Reported-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48bc-k_-1fh0dZpAhp_LiR5MfEX9haystmoBboR_4czCQ@mail.gmail.com
2023-09-01 14:21:27 +02:00
Alvaro Herrera
9b581c5341
Disallow changing NO INHERIT status of a not-null constraint
It makes no sense to add a NO INHERIT not-null constraint to a child
table that already has one in that column inherited from its parent.
Disallow that, and add tests for the relevant cases.

Per complaint from Kyotaro Horiguchi.  I also used part of his proposed
patch.

Co-authored-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Co-authored-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/20230828.161658.1184657435220765047.horikyota.ntt@gmail.com
2023-08-29 19:19:24 +02:00
Alvaro Herrera
952db4979f
Perl: Remove useless lines 2023-08-29 18:25:09 +02:00
Peter Eisentraut
63956bed7b Rename logical_replication_mode to debug_logical_replication_streaming
The logical_replication_mode GUC is intended for testing and debugging
purposes, but its current name may be misleading and encourage users to make
unnecessary changes.

To avoid confusion, renaming the GUC to a less misleading name
debug_logical_replication_streaming that casual users are less likely to mistakenly
assume needs to be modified in a regular logical replication setup.

Author: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Reviewed-by: Peter Smith <smithpb2250@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/d672d774-c44b-6fec-f993-793e744f169a%40eisentraut.org
2023-08-29 15:19:56 +02:00
Peter Eisentraut
648c72956f Convert encrypted SSL test keys to PKCS#8 format
OpenSSL in FIPS mode rejects several encrypted private keys used in
the test suites ssl and ssl_passphrase_callback.  This is because they
are in a "traditional" OpenSSL format that uses MD5 for key
generation.  The fix is to convert them to the more standard PKCS#8
format that uses SHA1 for key derivation.

This commit contains the converted keys, with the conversion done like
this:

openssl pkcs8 -topk8 -in src/test/modules/ssl_passphrase_callback/server.key -passin pass:FooBaR1 -out src/test/modules/ssl_passphrase_callback/server.key.new -passout pass:FooBaR1
mv src/test/modules/ssl_passphrase_callback/server.key.new src/test/modules/ssl_passphrase_callback/server.key

etc., as well as updated build rules to generate the keys in the new
format if they need to be regenerated.

Reviewed-by: Jacob Champion <jchampion@timescale.com>
Discussion: https://www.postgresql.org/message-id/flat/64de784b-8833-e055-3bd4-7420e6675351%40eisentraut.org
2023-08-28 07:37:43 +02:00
Michael Paquier
617f9b7d4b Tighten unit parsing in internal values
Interval values now generate an error when the user has multiple
consecutive units or a unit without a value.  Previously, it was
possible to specify multiple units consecutively which is contrary to
what the documentation allows, so it was possible to finish with
confusing interval values.

This is a follow-up of the work done in 165d581f14.

Author: Joseph Koshakow
Reviewed-by: Jacob Champion, Gurjeet Singh, Reid Thompson
Discussion: https://postgr.es/m/CAAvxfHd-yNO+XYnUxL=GaNZ1n+eE0V-oE0+-cC1jdjdU0KS3iw@mail.gmail.com
2023-08-28 14:27:17 +09:00
Michael Paquier
165d581f14 Tighten handling of "ago" in interval values
This commit Restrict the unit "ago" to only appear at the end of the
interval.  According to the documentation, a direction can only be
defined at the end of an interval, but it was possible to define it in
the middle of the string or define it multiple times.

In spirit, this is similar to the error handling improvements done in
5b3c595355 or bcc704b524.

Author: Joseph Koshakow
Reviewed-by: Jacob Champion, Gurjeet Singh, Reid Thompson
Discussion: https://postgr.es/m/CAAvxfHd-yNO+XYnUxL=GaNZ1n+eE0V-oE0+-cC1jdjdU0KS3iw@mail.gmail.com
2023-08-28 13:49:55 +09:00
Michael Paquier
e48b19c5db Generate new LOG for "trust" connections under log_connections
Adding an extra LOG for connections that have not set an authn ID, like
when the "trust" authentication method is used, is useful for audit
purposes.

A couple of TAP tests for SSL and authentication need to be tweaked to
adapt to this new LOG generated, as some scenarios expected no logs but
they now get a hit.

Reported-by: Shaun Thomas
Author: Jacob Champion
Reviewed-by: Robert Haas, Michael Paquier
Discussion: https://postgr.es/m/CAFdbL1N7-GF-ZXKaB3XuGA+CkSmnjFvqb8hgjMnDfd+uhL2u-A@mail.gmail.com
2023-08-26 20:11:19 +09:00
Andres Freund
1a4fd77db8 Avoid non-POSIX cp flags
Commit 252dcb32 used cp -a, but apparently Solaris doesn't like that.  Use cp
-RPp instead.

Author: Thomas Munro <thomas.munro@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CA+hUKGL10AoQVMMqgOJ8CTjoz9MLidD8ik2e8PibzLNMz0+aRg@mail.gmail.com
2023-08-25 06:43:37 -07:00
Alvaro Herrera
98976d0ce0
Rename test table to avoid cs_CZ locale problem
Per buildfarm member Hippopotamus
2023-08-25 14:06:13 +02:00
Alvaro Herrera
b0e96f3119
Catalog not-null constraints
We now create contype='n' pg_constraint rows for not-null constraints.

We propagate these constraints to other tables during operations such as
adding inheritance relationships, creating and attaching partitions and
creating tables LIKE other tables.  We also spawn not-null constraints
for inheritance child tables when their parents have primary keys.
These related constraints mostly follow the well-known rules of
conislocal and coninhcount that we have for CHECK constraints, with some
adaptations: for example, as opposed to CHECK constraints, we don't
match not-null ones by name when descending a hierarchy to alter it,
instead matching by column name that they apply to.  This means we don't
require the constraint names to be identical across a hierarchy.

For now, we omit them for system catalogs.  Maybe this is worth
reconsidering.  We don't support NOT VALID nor DEFERRABLE clauses
either; these can be added as separate features later (this patch is
already large and complicated enough.)

psql shows these constraints in \d+.

pg_dump requires some ad-hoc hacks, particularly when dumping a primary
key.  We now create one "throwaway" not-null constraint for each column
in the PK together with the CREATE TABLE command, and once the PK is
created, all those throwaway constraints are removed.  This avoids
having to check each tuple for nullness when the dump restores the
primary key creation.

pg_upgrading from an older release requires a somewhat brittle procedure
to create a constraint state that matches what would be created if the
database were being created fresh in Postgres 17.  I have tested all the
scenarios I could think of, and it works correctly as far as I can tell,
but I could have neglected weird cases.

This patch has been very long in the making.  The first patch was
written by Bernd Helmle in 2010 to add a new pg_constraint.contype value
('n'), which I (Álvaro) then hijacked in 2011 and 2012, until that one
was killed by the realization that we ought to use contype='c' instead:
manufactured CHECK constraints.  However, later SQL standard
development, as well as nonobvious emergent properties of that design
(mostly, failure to distinguish them from "normal" CHECK constraints as
well as the performance implication of having to test the CHECK
expression) led us to reconsider this choice, so now the current
implementation uses contype='n' again.  During Postgres 16 this had
already been introduced by commit e056c557ae, but there were some
problems mainly with the pg_upgrade procedure that couldn't be fixed in
reasonable time, so it was reverted.

In 2016 Vitaly Burovoy also worked on this feature[1] but found no
consensus for his proposed approach, which was claimed to be closer to
the letter of the standard, requiring an additional pg_attribute column
to track the OID of the not-null constraint for that column.
[1] https://postgr.es/m/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com

Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Author: Bernd Helmle <mailings@oopsware.de>
Reviewed-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com>
2023-08-25 13:31:24 +02:00
Andres Freund
252dcb3239 Use "template" data directory in tests
When running all (or just many) of our tests, a significant portion of both
CPU time and IO is spent running initdb. Most of those initdb runs don't
specify any options influencing properties of the created data directory.

Avoid most of that overhead by creating a "template" data directory, alongside
the temporary installation. Instead of running initdb, pg_regress and tap
tests can copy that data directory. When a tap test specifies options to
initdb, the template data directory is not used. That could be relaxed for
some options, but it's not clear it's worth the effort.

There unfortunately is some duplication between pg_regress.c and Cluster.pm,
but there are no easy ways of sharing that code without introducing additional
complexity.

Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/20220120021859.3zpsfqn4z7ob7afz@alap3.anarazel.de
2023-08-24 14:38:02 -07:00
Alvaro Herrera
3da13a6257
Add test for inherited CHECK constraint drop
This code is insufficiently covered by tests, so add a few small test
cases to immortalize its behavior before it gets rewritten completely by
the project to catalog NOT NULL constraints.
2023-08-24 16:51:43 +02:00
Peter Eisentraut
01226c682c Avoid use of Perl getprotobyname
getprotobyname returns undefined on some CI machines.  It's not clear
why.  The code overall still works, but it raises a warning.

In PostgreSQL C code, we always call socket() with 0 for the protocol
argument, so we should be able to do the same in Perl (since the Perl
documentation says that the arguments of the socket function are the
same as in C).  So do that, to avoid the issue.

Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://www.postgresql.org/message-id/flat/06f899fd-1826-05ab-42d6-adeb1fd5e200%40eisentraut.org
2023-08-23 20:48:48 +02:00