dlopen() is in SUSv2 and all targeted Unix systems have it. We still
need replacement functions for Windows, but we don't need a configure
probe for that.
Since it's no longer needed by other operating systems, rename dlopen.c
to win32dlopen.c and move the declarations into win32_port.h.
Likewise, the macros RTLD_NOW and RTLD_GLOBAL now only need to be
defined on Windows, since all targeted Unix systems have 'em.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CA+hUKGJ3LHeP9w5Fgzdr4G8AnEtJ=z=p6hGDEm4qYGEUX5B6fQ@mail.gmail.com
The test is proving to be unreliable in the buildfarm, and we neither
agree on how best to fix it nor have time to do so before the upcoming
release. So for now, put things back to the way they were before commit
d498e052b4.
Discussion: http://postgr.es/m/3628089.1659640252@sss.pgh.pa.us
Adjusting this function was overlooked in commit 94aa7cc5f. The only
visible symptom (so far) is that INSERT ... ON CONFLICT could go into
an endless loop when inserting a null that has a conflict.
Richard Guo and Tom Lane, per bug #17558 from Andrew Kesper
Discussion: https://postgr.es/m/17558-3f6599ffcf52fd4a@postgresql.org
Ordinarily the functions called in this loop ought to have plenty
of CFIs themselves; but we've now seen a case where no such CFI is
reached, making the loop uninterruptible. Even though that's from
a recently-introduced bug, it seems prudent to install a CFI at
the loop level in all branches.
Per discussion of bug #17558 from Andrew Kesper (an actual fix for
that bug will follow).
Discussion: https://postgr.es/m/17558-3f6599ffcf52fd4a@postgresql.org
Remove the test case added by commit fac1b470a, which never actually
worked to expose the problem it claimed to test. Replace it with
a case that does expose the problem, and also covers the SRF-not-
at-the-top deficiency repaired in 1aa8dad41.
Richard Guo, with some editorialization by me
Discussion: https://postgr.es/m/17564-c7472c2f90ef2da3@postgresql.org
The use of "we" when referring to the active backend might be
misunderstood, so rephrase to make it clearer who is performing
the actions discussed in the comment.
Author: Junwang Zhao <zhjwpku@gmail.com>
Reviewed-by: Erikjan Rijkers <er@xs4all.nl>
Reviewed-by: Robert Treat <rob@xzilla.net>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAEG8a3LRSMqkvjiURiJoSi4aGWORpiXUmUfQQK5PaD6WfPzu3w@mail.gmail.com
SSE2 vector instructions are part of the spec for the 64-bit x86
architecture. Until now we have relied on the compiler to autovectorize
in some limited situations, but some useful coding idioms can only be
expressed explicitly via compiler intrinsics. To this end, add a header
that defines USE_SSE2 where available. While x86-only for now, we can
add other architectures in the future. This will also be the intended
place for helper functions that use vector operations.
Reviewed by Nathan Bossart and Masahiko Sawada
Discussion: https://www.postgresql.org/message-id/CAFBsxsE2G_H_5Wbw%2BNOPm70-BK4xxKf86-mRzY%3DL2sLoQqM%2B-Q%40mail.gmail.com
Commit fac1b470a thought we could check for set-returning functions
by testing only the top-level node in an expression tree. This is
wrong in itself, and to make matters worse it encouraged others
to make the same mistake, by exporting tlist.c's special-purpose
IS_SRF_CALL() as a widely-visible macro. I can't find any evidence
that anyone's taken the bait, but it was only a matter of time.
Use expression_returns_set() instead, and stuff the IS_SRF_CALL()
genie back in its bottle, this time with a warning label. I also
added a couple of cross-reference comments.
After a fair amount of fooling around, I've despaired of making
a robust test case that exposes the bug reliably, so no test case
here. (Note that the test case added by fac1b470a is itself
broken, in that it doesn't notice if you remove the code change.
The repro given by the bug submitter currently doesn't fail either
in v15 or HEAD, though I suspect that may indicate an unrelated bug.)
Per bug #17564 from Martijn van Oosterhout. Back-patch to v13,
as the faulty patch was.
Discussion: https://postgr.es/m/17564-c7472c2f90ef2da3@postgresql.org
The sto_using_cursor and sto_using_select tests were coded to exercise
every permutation of their test steps, but AFAICS there is no value in
exercising more than one. This matters because each permutation costs
about six seconds, thanks to the "pg_sleep(6)". Perhaps we could
reduce that, but the useless permutations seem worth getting rid of
in any case. (Note that sto_using_hash_index got it right already.)
While here, clean up some other sloppiness such as an unused table.
This doesn't make too much difference in interactive testing, since the
wasted time is typically masked by parallelization with other tests.
However, the buildfarm runs this as a serial step, which means we can
expect to shave ~40 seconds from every buildfarm run. That makes it
worth back-patching.
Discussion: https://postgr.es/m/2515192.1659454702@sss.pgh.pa.us
The TAP tests for logical replication in src/test/subscription are using
the following code in many places to make sure that the subscription is
synchronized with the publisher:
$node_publisher->wait_for_catchup('tap_sub');
$node_subscriber->poll_query_until('postgres',
qq[SELECT count(1) = 0
FROM pg_subscription_rel
WHERE srsubstate NOT IN ('r', 's')]);
The new function wait_for_subscription_sync() can be used to replace the
above code. This eliminates duplicated code and makes it easier to write
future tests.
Author: Masahiko Sawada
Reviewed by: Amit Kapila, Shi yu
Discussion: https://postgr.es/m/CAD21AoC-fvAkaKHa4t1urupwL8xbAcWRePeETvshvy80f6WV1A@mail.gmail.com
Previously, a byte with the high bit set was just transmitted
as-is by charin() and charout(). This is problematic if the
database encoding is multibyte, because the result of charout()
won't be validly encoded, which breaks various stuff that
expects all text strings to be validly encoded. We've
previously decided to enforce encoding validity rather than try
to individually harden each place that might have a problem with
such strings, so it's time to do something about "char".
To fix, represent high-bit-set characters as \ooo (backslash
and three octal digits), following the ancient "escape" format
for bytea. charin() will continue to accept the old way as well,
though that is only reachable in single-byte encodings.
Add some test cases just so there is coverage for this code.
We'll otherwise leave this question undocumented as it was before,
because we don't really want to encourage end-user use of "char".
For the moment, back-patch into v15 so that this change appears
in 15beta3. If there's not great pushback we should consider
absorbing this change into the older branches.
Discussion: https://postgr.es/m/2318797.1638558730@sss.pgh.pa.us
ORDER BY / DISTINCT aggreagtes have, since implemented in Postgres, been
executed by always performing a sort in nodeAgg.c to sort the tuples in
the current group into the correct order before calling the transition
function on the sorted tuples. This was not great as often there might be
an index that could have provided pre-sorted input and allowed the
transition functions to be called as the rows come in, rather than having
to store them in a tuplestore in order to sort them once all the tuples
for the group have arrived.
Here we change the planner so it requests a path with a sort order which
supports the most amount of ORDER BY / DISTINCT aggregate functions and
add new code to the executor to allow it to support the processing of
ORDER BY / DISTINCT aggregates where the tuples are already sorted in the
correct order.
Since there can be many ORDER BY / DISTINCT aggregates in any given query
level, it's very possible that we can't find an order that suits all of
these aggregates. The sort order that the planner chooses is simply the
one that suits the most aggregate functions. We take the most strictly
sorted variation of each order and see how many aggregate functions can
use that, then we try again with the order of the remaining aggregates to
see if another order would suit more aggregate functions. For example:
SELECT agg(a ORDER BY a),agg2(a ORDER BY a,b) ...
would request the sort order to be {a, b} because {a} is a subset of the
sort order of {a,b}, but;
SELECT agg(a ORDER BY a),agg2(a ORDER BY c) ...
would just pick a plan ordered by {a} (we give precedence to aggregates
which are earlier in the targetlist).
SELECT agg(a ORDER BY a),agg2(a ORDER BY b),agg3(a ORDER BY b) ...
would choose to order by {b} since two aggregates suit that vs just one
that requires input ordered by {a}.
Author: David Rowley
Reviewed-by: Ronan Dunklau, James Coleman, Ranier Vilela, Richard Guo, Tom Lane
Discussion: https://postgr.es/m/CAApHDvpHzfo92%3DR4W0%2BxVua3BUYCKMckWAmo-2t_KiXN-wYH%3Dw%40mail.gmail.com
The select_outer_pathkeys_for_merge function made an attempt to build the
merge join pathkeys in the same order as query_pathkeys. This was done as
it may have led to no sort being required for an ORDER BY or GROUP BY
clause in the upper planner. However, this restriction seems overly
strict as it required that we match the query_pathkeys entirely or we
don't bother putting the merge join pathkeys in that order.
Here we relax this rule so that we use a prefix of the query_pathkeys
providing that prefix matches all of the join quals. This may provide the
upper planner with partially sorted input which will allow the use of
incremental sorts instead of full sorts.
Author: David Rowley
Reviewed-by: Richard Guo
Discussion: https://postgr.es/m/CAApHDvrtZu0PHVfDPFM4Yx3jNR2Wuwosv+T2zqa7LrhhBr2rRg@mail.gmail.com
Here we add code which detects when ExecFindPartition() continually finds
the same partition and add a caching layer to improve partition lookup
performance for such cases.
Both RANGE and LIST partitioned tables traditionally require a binary
search for the set of Datums that a partition needs to be found for. This
binary search is commonly visible in profiles when bulk loading into a
partitioned table. Here we aim to reduce the overhead of bulk-loading
into partitioned tables for cases where many consecutive tuples belong to
the same partition and make the performance of this operation closer to
what it is with a traditional non-partitioned table.
When we find the same partition 16 times in a row, the next search will
result in us simply just checking if the current set of values belongs to
the last found partition. For LIST partitioning we record the index into
the PartitionBoundInfo's datum array. This allows us to check if the
current Datum is the same as the Datum that was last looked up. This
means if any given LIST partition supports storing multiple different
Datum values, then the caching only works when we find the same value as
we did the last time. For RANGE partitioning we simply check if the given
Datums are in the same range as the previously found partition.
We store the details of the cached partition in PartitionDesc (i.e.
relcache) so that the cached values are maintained over multiple
statements.
No caching is done for HASH partitions. The majority of the cost in HASH
partition lookups are in the hashing function(s), which would also have to
be executed if we were to try to do caching for HASH partitioned tables.
Since most of the cost is already incurred, we just don't bother. We also
don't do any caching for LIST partitions when we continually find the
values being looked up belong to the DEFAULT partition. We've no
corresponding index in the PartitionBoundInfo's datum array for this case.
We also don't cache when we find the given values match to a LIST
partitioned table's NULL partition. This is so cheap that there's no
point in doing any caching for this. We also don't cache for a RANGE
partitioned table's DEFAULT partition.
There have been a number of different patches submitted to improve
partition lookups. Hou, Zhijie submitted a patch to detect when the value
belonging to the partition key column(s) were constant and added code to
cache the partition in that case. Amit Langote then implemented an idea
suggested by me to remember the last found partition and start to check if
the current values work for that partition. The final patch here was
written by me and was done by taking many of the ideas I liked from the
patches in the thread and redesigning other aspects.
Discussion: https://postgr.es/m/OS0PR01MB571649B27E912EA6CC4EEF03942D9%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Author: Amit Langote, Hou Zhijie, David Rowley
Reviewed-by: Amit Langote, Hou Zhijie
I thought commit fd96d14d9 had plugged all the holes of this sort,
but no, function RTEs could produce oversize tuples too, either
via long coldeflists or just from multiple functions in one RTE.
(I'm pretty sure the other variants of base RTEs aren't a problem,
because they ultimately refer to either a table or a sub-SELECT,
whose widths are enforced elsewhere. But we explicitly allow join
RTEs to be overwidth, as long as you don't try to form their
tuple result.)
Per further discussion of bug #17561. As before, patch all branches.
Discussion: https://postgr.es/m/17561-80350151b9ad2ad4@postgresql.org
errno was not reported correctly after attempting to clone a file,
leading to incorrect error reports. While scanning through the code, I
have not noticed any similar mistakes.
Error introduced in 3a769d8.
Author: Justin Pryzby
Discussion: https://postgr.es/m/20220731134135.GY15006@telsasoft.com
Backpatch-through: 12
This commit adds some test coverage for ee79647 (prevent BASE_BACKUP
from running in the middle of another base backup) and b24b2be
(BASE_BACKUP cancellation followed by pg_backup_start), caused by the
interactions of replication and SQL commands in a logical replication
connection in a WAL sender.
The second test uses a design close to what has been introduced in
0475a97f, where BASE_BACKUP is throttled to give enough room for a
cancellation, though this time we rely on psql with multiple -c
switches to keep a connection around for the second query.
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/Ys/NCI4Eo9300GnQ@paquier.xyz
In the short time this function has existed, it's already proven to be
a nontrivial maintenance burden, since it has to be updated whenever a
node tag is added or removed. Although in principle we could now
automate that, I see little justification for having such functionality
here at all. The function is only being applied to utility statements,
for which we already have infrastructure for obtaining string names.
Moreover, that infrastructure produces already-familiar-to-users names,
unlike nodetag_to_string().
So, remove this function and use the existing infrastructure instead.
That saves over a thousand lines of largely-unreachable code.
Back-patch to v15 where this code came in. Although it seems unlikely
that v15's nodetag list will change anymore, we might as well keep the
two branches looking and acting alike; otherwise back-patching any
test-results changes in this area will be painful.
Discussion: https://postgr.es/m/843818.1659218928@sss.pgh.pa.us
These two new options can be used to either process all tables in
specific schemas or to skip processing all tables in specific
schemas. This change also refactors the handling of invalid
combinations of command-line options to a new helper function.
Author: Gilles Darold
Reviewed-by: Justin Pryzby, Nathan Bossart and Michael Paquier.
Discussion: https://postgr.es/m/929fbf3c-24b8-d454-811f-1d5898ab3e91%40migops.com
The code tried to access ARR_DIMS(v)[0] and ARR_LBOUND(v)[0]
whether or not those values exist. This made the range check
on the "n" argument unstable --- it might or might not fail, and
if it did it would report garbage for the allowed upper limit.
These bogus accesses would probably annoy Valgrind, and if you
were very unlucky even lead to SIGSEGV.
Report and fix by Martin Kalcher. Back-patch to v14 where this
function was added.
Discussion: https://postgr.es/m/baaeb413-b8a8-4656-5757-ef347e5ec11f@aboutsource.net
These flavors of ALTER TABLE were already shaped to report the
ObjectAddress of the partition attached or detached, but this data was
not added to what is collected for event triggers. The tests of
test_ddl_deparse are updated to show the modification in the data
reported.
Author: Hou Zhijie
Reviewed-by: Álvaro Herrera, Amit Kapila, Hayato Kuroda, Michael Paquier
Discussion: https://postgr.es/m/OS0PR01MB571626984BD099DADF53F38394899@OS0PR01MB5716.jpnprd01.prod.outlook.com
This module is expanded to track the description of the objects changed
in the subcommands of ALTER TABLE by reworking the function
get_altertable_subcmdtypes() (now named get_altertable_subcmdinfo) used
in the event trigger of the test. It now returns a set of rows made of
(subcommand type, object description) instead of a text array with only
the information about the subcommand type.
The tests have been lacking a lot of the subcommands added to
AlterTableType over the years. All the missing subcommands are added,
and the code is now structured so as the addition of a new subcommand
is detected by removing the default clause used in the switch for the
subcommand types.
The coverage of the module is increased from roughly 30% to 50%. More
could be done but this is already a nice improvement.
Author: Michael Paquier, Hou Zhijie
Reviewed-by: Álvaro Herrera, Amit Kapila, Hayato Kuroda
Discussion: https://postgr.es/m/OS0PR01MB571626984BD099DADF53F38394899@OS0PR01MB5716.jpnprd01.prod.outlook.com
Two callers of generate_useful_gather_paths were testing the wrong
thing when deciding whether to call that function: they checked for
being at the top of the current join subproblem, rather than being at
the actual top join. This'd result in failing to construct parallel
paths for a sub-join for which they might be useful.
While set_rel_pathlist() isn't actively broken, it seems best to
make its identical-in-intention test for this be like the other two.
This has been wrong all along, but given the lack of field complaints
I'm hesitant to back-patch into stable branches; we usually prefer
to avoid non-bug-fix changes in plan choices in minor releases.
It seems not too late for v15 though.
Richard Guo, reviewed by Antonin Houska and Tom Lane
Discussion: https://postgr.es/m/CAMbWs4-mH8Zf87-w+3P2J=nJB+5OyicO28ia9q_9o=Lamf_VHg@mail.gmail.com
It's allowed for an installation to remove postgresql.auto.conf,
so don't rely on that being present. Instead probe whether we can
read postmaster.pid. (If you've removed that, you broke the data
directory's multiple-postmaster interlock, not to mention pg_ctl.)
Per gripe from Michael Paquier.
Discussion: https://postgr.es/m/YuSZTsoBMObyY+vT@paquier.xyz
Instead of using command_ok() to run psql, use safe_psql(). wrasse
isn't happy, and it be because of failure to pass -X to the psql
invocation, which safe_psql() will do automatically.
Since safe_psql() returns standard output instead of writing it to
a file, this requires some changes to the incantation for running
'diff'.
Test against the 'regression' database rather than 'postgres' so
we test more than just one table. That also means we need to record
the horizons later, after the test does "VACUUM FULL pg_largeobject".
Add an ORDER BY clause to the horizon query for stability.
Patch by me, reviewed by Tom Lane.
Discussion: http://postgr.es/m/CA+TgmoaGBbpzgu3=du1f9zDUbkfycO0y=_uWrLFy=KKEqXWeLQ@mail.gmail.com
The new test is from commit 9e4f914b5e.
With this setting messages have SQL error numbers included, so that
needs to be provided for in the pattern looked for.
We must issue the TRUNCATE command first and update relfrozenxid
and relminmxid afterward; otherwise, TRUNCATE overwrites the
previously-set values.
Add a test case like I should have done the first time.
Per buildfarm report from TestUpgradeXversion.pm, by way of Tom
Lane.
There wasn't an especially nice way to read all of a file while
passing missing_ok = true. Add an additional overloaded variant
to support that use-case.
While here, refactor the C code to avoid a rats-nest of PG_NARGS
checks, instead handling the argument collection in the outer
wrapper functions. It's a bit longer this way, but far more
straightforward.
(Upon looking at the code coverage report for genfile.c, I was
impelled to also add a test case for pg_stat_file() -- tgl)
Kyotaro Horiguchi
Discussion: https://postgr.es/m/20220607.160520.1984541900138970018.horikyota.ntt@gmail.com
A RowExpr with more than MaxTupleAttributeNumber columns would fail at
execution anyway, since we cannot form a tuple datum with more than that
many columns. While heap_form_tuple() has a check for too many columns,
it emerges that there are some intermediate bits of code that don't
check and can be driven to failure with sufficiently many columns.
Checking this at parse time seems like the most appropriate place to
install a defense, since we already check SELECT list length there.
While at it, make the SELECT-list-length error use the same errcode
(TOO_MANY_COLUMNS) as heap_form_tuple does, rather than the generic
PROGRAM_LIMIT_EXCEEDED.
Per bug #17561 from Egor Chindyaskin. The given test case crashes
in all supported branches (and probably a lot further back),
so patch all.
Discussion: https://postgr.es/m/17561-80350151b9ad2ad4@postgresql.org
The earlier commit used pg_class.relfilenode where it should have
used pg_class.oid. This could lead to emitting an UPDATE statement
into the dump that would update nothing (or the wrong thing) when
executed in the new cluster, resulting in relfrozenxid and
relminmxid being improperly carried forward for pg_largeobject.
Noticed by Dilip Kumar.
Discussion: http://postgr.es/m/CAFiTN-ty1Gzs6stk2vt9BJiq0m0hzf=aPnh3a-4Z3Tk5GzoENw@mail.gmail.com
On FreeBSD, the new test fails due to a WAL file being removed before
the standby has had the chance to copy it. Fix by adding a replication
slot to prevent the removal until after the standby has connected.
Author: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reported-by: Matthias van de Meent <boekewurm+postgres@gmail.com>
Discussion: https://postgr.es/m/CAEze2Wj5nau_qpjbwihvmXLfkAWOZ5TKdbnqOc6nKSiRJEoPyQ@mail.gmail.com
Commit 9a974cbcba arranged to preserve
the relfilenode of user tables across pg_upgrade, but failed to notice
that pg_upgrade treats pg_largeobject as a user table and thus it needs
the same treatment. Otherwise, large objects will appear to vanish
after a pg_upgrade.
Commit d498e052b4 fixed this problem
by teaching pg_dump to UPDATE pg_class.relfilenode for pg_largeobject
and its index. However, because an UPDATE on the catalog rows doesn't
change anything on disk, this can leave stray files behind in the new
cluster. They will normally be empty, but it's a little bit untidy.
Hence, this commit arranges to do the same thing using DDL. Specifically,
it makes TRUNCATE work for the pg_largeobject catalog when in
binary-upgrade mode, and it then uses that command in binary-upgrade
dumps as a way of setting pg_class.relfilenode for pg_largeobject and
its index. That way, the old files are removed from the new cluster.
Discussion: http://postgr.es/m/CA+TgmoYYMXGUJO5GZk1-MByJGu_bB8CbOL6GJQC8=Bzt6x6vDg@mail.gmail.com
In the initial data sort, if the bucket numbers are the same then
next sort on the hash value. Because index pages are kept in
hash value order, this gains a little speed by allowing the
eventual tuple insertions to be done sequentially, avoiding repeated
data movement within PageAddItem. This seems to be good for overall
speedup of 5%-9%, depending on the incoming data.
Simon Riggs, reviewed by Amit Kapila
Discussion: https://postgr.es/m/CANbhV-FG-1ZNMBuwhUF7AxxJz3u5137dYL-o6hchK1V_dMw86g@mail.gmail.com
Commit b0a55e4329 missed a few places
where we are referring to the number used as a part of the relation
filename as an "OID". We now want to call that a "RelFileNumber".
Some of these places actually made it sound like the OID in question
is pg_class.oid rather than pg_class.relfilenode, which is especially
good to clean up.
Dilip Kumar with some editing by me.
Crash recovery on standby may encounter missing directories
when replaying database-creation WAL records. Prior to this
patch, the standby would fail to recover in such a case;
however, the directories could be legitimately missing.
Consider the following sequence of commands:
CREATE DATABASE
DROP DATABASE
DROP TABLESPACE
If, after replaying the last WAL record and removing the
tablespace directory, the standby crashes and has to replay the
create database record again, crash recovery must be able to continue.
A fix for this problem was already attempted in 49d9cfc68b, but it
was reverted because of design issues. This new version is based
on Robert Haas' proposal: any missing tablespaces are created
during recovery before reaching consistency. Tablespaces
are created as real directories, and should be deleted
by later replay. CheckRecoveryConsistency ensures
they have disappeared.
The problems detected by this new code are reported as PANIC,
except when allow_in_place_tablespaces is set to ON, in which
case they are WARNING. Apart from making tests possible, this
gives users an escape hatch in case things don't go as planned.
Author: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Author: Asim R Praveen <apraveen@pivotal.io>
Author: Paul Guo <paulguo@gmail.com>
Reviewed-by: Anastasia Lubennikova <lubennikovaav@gmail.com> (older versions)
Reviewed-by: Fujii Masao <masao.fujii@oss.nttdata.com> (older versions)
Reviewed-by: Michaël Paquier <michael@paquier.xyz>
Diagnosed-by: Paul Guo <paulguo@gmail.com>
Discussion: https://postgr.es/m/CAEET0ZGx9AvioViLf7nbR_8tH9-=27DN5xWJ2P9-ROH16e4JUA@mail.gmail.com
On Windows with MSVC, get_dirent_type() was recently made to return
DT_LNK for junction points by commit 9d3444dc, which fixed some
defective dirent.c code.
On Windows with Cygwin, get_dirent_type() already worked for symlinks,
as it does on POSIX systems, because Cygwin has its own fake symlinks
that behave like POSIX (on closer inspection, Cygwin's dirent has the
BSD d_type extension but it's probably always DT_UNKNOWN, so we fall
back to lstat(), which understands Cygwin symlinks with S_ISLNK()).
On Windows with MinGW/MSYS, we need extra code, because the MinGW
runtime has its own readdir() without d_type, and the lstat()-based
fallback has no knowledge of our convention for treating junctions as
symlinks.
Back-patch to 14, where get_dirent_type() landed.
Reported-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/b9ddf605-6b36-f90d-7c30-7b3e95c46276%40dunslane.net
The catalog contents haven't changed, but it's good to make clear
that initdb is required. Changing RELMAPPER_FILEMAGIC would be more
appropriate, but that doesn't actually produce a useful diagnostic,
so cheat by doing this instead.
Discussion: http://postgr.es/m/20220727171939.6ixixqcjt5riil2o@alvherre.pgsql
Commit d8cd0c6c95 introduced a file
rename that could fail on Windows, probably due to other backends
having an open file handle to the old file of the same name.
Re-arrange the locking slightly to prevent that, by making sure the
open() and close() run while we hold the lock.
Thomas Munro. I added an explanatory comment.
Discussion: https://postgr.es/m/CA%2BhUKGLZtCTgp4NTWV-wGbR2Nyag71%3DEfYTKjDKnk%2BfkhuFMHw%40mail.gmail.com
GetSubscriptionRelations() and GetSubscriptionNotReadyRelations() share
mostly the same code, which scans pg_subscription_rel and fetches all
the relations of a given subscription. The only difference is that the
second routine looks for all the relations not in a ready state. This
commit refactors the code to use a single routine, shaving a bit of
code.
Author: Vignesh C
Reviewed-By: Kyotaro Horiguchi, Amit Kapila, Michael Paquier, Peter
Smith
Discussion: https://postgr.es/m/CALDaNm0eW-9g4G_EzHebnFT5zZoasWCS_EzZQ5BgnLZny9S=pg@mail.gmail.com
This commit puts the implementation of Tuple sort variants into the separate
file tuplesortvariants.c. That gives better separation of the code and
serves well as the demonstration that Tuple sort variant can be defined outside
of tuplesort.c.
Discussion: https://postgr.es/m/CAPpHfdvjix0Ahx-H3Jp1M2R%2B_74P-zKnGGygx4OWr%3DbUQ8BNdw%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Pavel Borisov, Maxim Orlov, Matthias van de Meent
Reviewed-by: Andres Freund, John Naylor
The new TuplesortPublic data structure contains the definition of
sort-variant-specific interface methods and the part of Tuple sort operation
state required by their implementations. This will let define Tuple sort
variants without knowledge of Tuplesortstate, that is without knowledge
of generic sort implementation guts.
Discussion: https://postgr.es/m/CAPpHfdvjix0Ahx-H3Jp1M2R%2B_74P-zKnGGygx4OWr%3DbUQ8BNdw%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Pavel Borisov, Maxim Orlov, Matthias van de Meent
Reviewed-by: Andres Freund, John Naylor
This commit puts some generic work away from sort-variant-specific function.
In particular, tuplesort_put*() now doesn't need to decrease available memory
and switch to sort context before calling puttuple_common(). writetup()
doesn't need to free SortTuple.tuple and increase available memory.
Discussion: https://postgr.es/m/CAPpHfdvjix0Ahx-H3Jp1M2R%2B_74P-zKnGGygx4OWr%3DbUQ8BNdw%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Pavel Borisov, Maxim Orlov, Matthias van de Meent
Reviewed-by: Andres Freund, John Naylor
Abbreviation code is very similar along tuplesort_put*() functions. This
commit unifies that code and puts it into puttuple_common(). tuplesort_put*()
functions differs in the abbreviation condition, so it has been added as an
argument to the puttuple_common() function.
Discussion: https://postgr.es/m/CAPpHfdvjix0Ahx-H3Jp1M2R%2B_74P-zKnGGygx4OWr%3DbUQ8BNdw%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Pavel Borisov, Maxim Orlov, Matthias van de Meent
Reviewed-by: Andres Freund, John Naylor
This commit is the preparation to move abbreviation logic into
puttuple_common(). The new removeabbrev function turns datum1 representation
of SortTuple's from the abbreviated key to the first column value. Therefore,
it encapsulates the differential part of abbreviation handling code in
tuplesort_put*() functions, making these functions similar.
Discussion: https://postgr.es/m/CAPpHfdvjix0Ahx-H3Jp1M2R%2B_74P-zKnGGygx4OWr%3DbUQ8BNdw%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Pavel Borisov, Maxim Orlov, Matthias van de Meent
Reviewed-by: Andres Freund, John Naylor
It's currently unclear how do we split functionality between
Tuplesortstate.copytup() function and tuplesort_put*() functions.
For instance, copytup_index() and copytup_datum() return error while
tuplesort_putindextuplevalues() and tuplesort_putdatum() do their work.
This commit removes Tuplesortstate.copytup() altogether, putting the
corresponding code into tuplesort_put*().
Discussion: https://postgr.es/m/CAPpHfdvjix0Ahx-H3Jp1M2R%2B_74P-zKnGGygx4OWr%3DbUQ8BNdw%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Pavel Borisov, Maxim Orlov, Matthias van de Meent
Reviewed-by: Andres Freund, John Naylor
XLogRecordBlockHeader, the header holding the information for the data
related to a block, tracks the length of the data appended to the WAL
record with data_length (uint16). This limitation in size was not
enforced by the public routine in charge of registering the data
assembled later to form the WAL record inserted, XLogRegisterBufData().
Incorrectly used, it could lead to the generation of records with some
of its data overflowed. This commit adds some safeguards to prevent
that for the block data, complaining immediately if attempting to add to
a record block information with a size larger than UINT16_MAX, which is
the limit implied by the internal logic.
Note that this also adjusts XLogRegisterData() and XLogRegisterBufData()
so as the length of the WAL record data given by the caller is unsigned,
matching with what gets stored in XLogRecData->len.
Extracted from a larger patch by the same author. The original patch
includes more protections when assembling a record in full that will be
looked at separately later.
Author: Matthias van de Meent
Reviewed-by: Andres Freund, Heikki Linnakangas, Michael Paquier, David
Zhang
Discussion: https://postgr.es/m/CAEze2WgGiw+LZt+vHf8tWqB_6VxeLsMeoAuod0N=ij1q17n5pw@mail.gmail.com
As before, we start by prepending one underscore (truncating the
base name if necessary). But if there is a conflict, then instead of
prepending more and more underscores, append an underscore and some
digits, in much the same way that ChooseRelationName does. While
the previous logic could be driven to fail by creating a lot of
types with long names differing only near the end, this version seems
certain enough to eventually succeed that we can remove the failure
code path that was there before.
While at it, undo 6df7a9698's decision to split this code out of
makeArrayTypeName. That wasn't actually accomplishing anything,
because no other function was using it --- and it would have been
wrong to do so. The convention that a prefix "_" means an array,
not something else, is too ancient to mess with.
Andrey Lepikhov and Dmitry Koval, reviewed by Masahiko Sawada and myself
Discussion: https://postgr.es/m/b84cd82c-cc67-198a-8b1c-60f44e1259ad@postgrespro.ru
The BoolGetDatum() call ended up in the wrong place. It should be
applied when we, err, want to convert a bool to a datum.
Thanks to Tom Lane for noticing this.
Discussion: http://postgr.es/m/2511599.1658861964@sss.pgh.pa.us
A bootstrap user who is not a superuser will still own many
important system objects, such as the pg_catalog schema, that
will likely allow that user to regain superuser status. Therefore,
allowing the superuser property to be removed from the superuser
creates a false perception of security where none exists.
Although removing superuser from the bootstrap user is also a bad idea
and should be considered unsupported in all released versions, no
back-patch, as this is a behavior change.
Discussion: http://postgr.es/m/CA+TgmoZirCwArJms_fgvLBFrC6b=HdxmG7iAhv+kt_=NBA7tEw@mail.gmail.com
We have a few commands that "can't run in a transaction block",
meaning that if they complete their processing but then we fail
to COMMIT, we'll be left with inconsistent on-disk state.
However, the existing defenses for this are only watertight for
simple query protocol. In extended protocol, we didn't commit
until receiving a Sync message. Since the client is allowed to
issue another command instead of Sync, we're in trouble if that
command fails or is an explicit ROLLBACK. In any case, sitting
in an inconsistent state while waiting for a client message
that might not come seems pretty risky.
This case wasn't reachable via libpq before we introduced pipeline
mode, but it's always been an intended aspect of extended query
protocol, and likely there are other clients that could reach it
before.
To fix, set a flag in PreventInTransactionBlock that tells
exec_execute_message to force an immediate commit. This seems
to be the approach that does least damage to existing working
cases while still preventing the undesirable outcomes.
While here, add some documentation to protocol.sgml that explicitly
says how to use pipelining. That's latent in the existing docs if
you know what to look for, but it's better to spell it out; and it
provides a place to document this new behavior.
Per bug #17434 from Yugo Nagata. It's been wrong for ages,
so back-patch to all supported branches.
Discussion: https://postgr.es/m/17434-d9f7a064ce2a88a3@postgresql.org
Presently, archive status files are durably renamed from .ready to
.done to indicate that a file has been archived. Persisting this
rename to disk accounts for a significant amount of the overhead
associated with archiving. While durably renaming the file
prevents re-archiving in most cases, archive commands and libraries
must already gracefully handle attempts to re-archive the last
archived file after a crash (e.g., a crash immediately after
archive_command exits but before the server renames the status
file).
This change reduces the amount of overhead associated with
archiving by using rename() instead of durable_rename() to rename
the archive status files. As a consequence, the server is more
likely to attempt to re-archive files after a crash, but as noted
above, archive commands and modules are already expected to handle
this. It is also possible that the server will attempt to re-
archive files that have been removed or recycled, but the archiver
already handles this, too.
Author: Nathan Bossart
Reviewed-by: Kyotaro Horiguchi, Fujii Masao
Discussion: https://postgr.es/m/20220222011948.GA3850532@nathanxps13
Since a2c8499, HbaFileName (default pg_hba.conf) was getting used
instead of IdentFileName (default pg_ident.conf) as the parent file to
use as reference when parsing the contents of pg_ident.conf, with
pg_ident.conf correctly opened, when feeding this information to
pg_ident_file_mappings. This had two consequences:
- On an I/O error when reading pg_ident.conf, the user would get an
ERROR message referring to pg_hba.conf and not pg_ident.conf.
- When reading an external file with a relative path using '@' in
pg_ident.conf, the directory used to look at the file to load would be
the base directory of pg_hba.conf rather than the one of pg_ident.conf,
leading to errors in pg_ident_file_mappings inconsistent with what gets
loaded at startup when pg_ident.conf and pg_hba.conf are located in
different directories.
This error only impacted the SQL view pg_ident_file_mappings that uses a
logic new to v15 to fill the view with the parsed information, not the
code paths loading these authentication files at startup.
Author: Julien Rouhaud
Discussion: https://postgr.es/m/20220726050402.vsr6fmz7rsgpmdz3@jrouhaud
Backpatch-through: 15
This addresses a couple of bugs in the REINDEX grammar, introduced by
83011ce:
- A name was never specified for DATABASE/SYSTEM, even if the query
included one. This caused such REINDEX queries to always work with any
object name, but we should complain if the object name specified does
not match the name of the database we are connected to. A test is added
for this case in the main regression test suite, provided by Álvaro.
- REINDEX SYSTEM CONCURRENTLY [name] was getting rejected in the
parser. Concurrent rebuilds are not supported for catalogs but the
error provided at execution time is more helpful for the user, and
allowing this flavor results in a simplification of the parsing logic.
- REINDEX DATABASE CONCURRENTLY was rebuilding the index in a
non-concurrent way, as the option was not being appended correctly in
the list of DefElems in ReindexStmt (REINDEX (CONCURRENTLY) DATABASE was
working fine. A test is added in the TAP tests of reindexdb for this
case, where we already have a REINDEX DATABASE CONCURRENTLY query
running on a small-ish instance. This relies on the work done in
2cbc3c1 for SYSTEM, but here we check if the OIDs of the index relations
match or not after the concurrent rebuild. Note that in order to get
this part to work, I had to tweak the tests so as the index OID and
names are saved separately. This change not affect the reliability or
of the coverage of the existing tests.
While on it, I have implemented a tweak in the grammar to reduce the
parsing by one branch, simplifying things even more.
Author: Michael Paquier, Álvaro Herrera
Discussion: https://postgr.es/m/YttqI6O64wDxGn0K@paquier.xyz
The setting controls tha maximum length of the header line in expanded
format output. Possible settings are full, column, page, or an integer.
the default is full, the current behaviour, and in this case the header
line is the length of the widest line of output. column causes the
header to be truncated to the width of the first column, page causes it
to be truncated to the width of the terminal page, and an integer causes
it to be truncated to that value. If the full value is less than the
page or integer value no truncation occurs. If given without an argument
this option prints its current setting.
Platon Pronko, somewhat modified by me.
Discussion: https://postgr.es/m/f03d38a3-db96-a56e-d1bc-dbbc80bbde4d@gmail.com
Previously we did this after InitPostgres, at a somewhat randomly chosen
place within PostgresMain. However, since commit a0ffa885e doing this
outside a transaction can cause a crash, if we need to check permissions
while replacing a placeholder GUC. (Besides which, a preloaded library
could itself want to do database access within _PG_init.)
To avoid needing an additional transaction start/end in every session,
move the process_session_preload_libraries call to within InitPostgres's
transaction. That requires teaching the code not to call it when
InitPostgres is called from somewhere other than PostgresMain, since
we don't want session_preload_libraries to affect background workers.
The most future-proof solution here seems to be to add an additional
flag parameter to InitPostgres; fortunately, we're not yet very worried
about API stability for v15.
Doing this also exposed the fact that we're currently honoring
session_preload_libraries in walsenders, even those not connected to
any database. This seems, at minimum, a POLA violation: walsenders
are not interactive sessions. Let's stop doing that.
(All these comments also apply to local_preload_libraries, of course.)
Per report from Gurjeet Singh (thanks also to Nathan Bossart and Kyotaro
Horiguchi for review). Backpatch to v15 where a0ffa885e came in.
Discussion: https://postgr.es/m/CABwTF4VEpwTHhRQ+q5MiC5ucngN-whN-PdcKeufX7eLSoAfbZA@mail.gmail.com
It incorrectly used GetBufferDescriptor instead of
GetLocalBufferDescriptor, causing it to not find the correct buffer in
most cases, and performing an out-of-bounds memory read in the corner
case that temp_buffers > shared_buffers.
It also bumped the usage-count on the buffer, even if it was
previously pinned. That won't lead to crashes or incorrect results,
but it's different from what the shared-buffer case does, and
different from the usual code in LocalBufferAlloc. Fix that too, and
make the code ordering match LocalBufferAlloc() more closely, so that
it's easier to verify that it's doing the same thing.
Currently, ReadRecentBuffer() is only used with non-temp relations, in
WAL redo, so the broken code is currently dead code. However, it could
be used by extensions.
Backpatch-through: 14
Discussion: https://www.postgresql.org/message-id/2d74b46f-27c9-fb31-7f99-327a87184cc0%40iki.fi
Reviewed-by: Thomas Munro, Zhang Mingli, Richard Guo
This commit removes two arguments "report" and "whichChkpt"
in ReadCheckpointRecord().
"report" is obviously useless because it's always true, i.e., there are
two callers of the function and they always specify true as "report".
Commit 1d919de5eb removed the only call with "report" = false.
"whichChkpt" indicated where the specified checkpoint location
came from, pg_control or backup_label. This information was used
to report different error messages depending on where the invalid
checkpoint record came from, when it was found.
But ReadCheckpointRecord() doesn't need to do that because
its callers already do that and users can still identify where
the invalid checkpoint record came from, by reading such log messages.
Also when "whichChkpt" was 0, the word "primary checkpoint" was used
in the log message and could confuse users because the concept of
primary and secondary checkpoints was already removed before.
These are why this commit removes "whichChkpt" argument.
Author: Fujii Masao
Reviewed-by: Bharath Rupireddy, Kyotaro Horiguchi
Discussion: https://postgr.es/m/fa2e12eb-81c3-0717-0272-755f8a81c8f2@oss.nttdata.com
sigwait() is in SUSv2 and all targeted Unix systems have it. An earlier
pre-standard function prototype existed on some older systems, but we
no longer need a workaround for that.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Greg Stark <stark@mit.edu>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/CA+hUKGJ3LHeP9w5Fgzdr4G8AnEtJ=z=p6hGDEm4qYGEUX5B6fQ@mail.gmail.com
getrusage() is in SUSv2 and all targeted Unix systems have it.
Note that POSIX only covers ru_utime and ru_stime and we rely on many
more fields without any kind of configure probe, but that predates this
commit.
The only supported system we need replacement code for now is Windows,
and that can be done without a configure probe.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Greg Stark <stark@mit.edu>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/CA+hUKGJ3LHeP9w5Fgzdr4G8AnEtJ=z=p6hGDEm4qYGEUX5B6fQ@mail.gmail.com
Commit e2f65f425 added contrib/pg_prewarm to the prerequisites for
running the src/test/recovery suite, but did not bother to update
the documentation about that.
We've long held the minimum at 3.80, but that's required more than
one workaround. Commit 0f39b70a6 broke it again, because it turns
out that exporting a target-specific variable didn't work in 3.80.
Considering that 3.81 is now old enough to get a driver's license,
and that the only remaining buildfarm member testing 3.80 (prairiedog)
is likely to be retired soon, let's just stop supporting 3.80.
Adjust docs and Makefile.global's minimum-version check to match.
There are a couple of comments in the Makefiles suggesting that
random things could be done differently after we desupport 3.80,
but I couldn't get excited about changing any of them right now.
Back-patch to v15, as 0f39b70a6 was.
Discussion: https://postgr.es/m/20220720172321.GL12702@telsasoft.com
The common recipe when TAP tests are disabled doesn't work, because the
libpq-specific recipe wants to define the PATH environment variable, so
the starting '@' is misinterpreted as part of the command instead of
silencing said command.
Fix by setting the environment variable in a way that doesn't interfere
with the recipe.
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/20220720172321.GL12702@telsasoft.com
The dependency logic failed to register a column-level dependency
when a view or rule contains a reference to a specific column of
the result of a function-returning-composite. That meant you could
drop the column from the composite type, causing trouble for future
executions of the view. We've known about this for years, but never
summoned the energy to actually fix it, instead installing various
low-level defenses to prevent crashing on references to dropped columns.
We had to do that to plug the hole in stable branches, where there might
be pre-existing broken references; but let's fix the root cause today.
To do that, add some logic (borrowed from get_rte_attribute_is_dropped)
to find_expr_references_walker, to check whether a Var referencing an
RTE_FUNCTION RTE is referencing a column of a composite type, and if
so add the proper dependency.
However ... it seems mighty unwise to remove said low-level defenses,
since there could be other bugs now or in the future that allow
reaching them. By the same token, letting those defenses go untested
seems unwise. Hence, rather than just dropping the associated test
cases, hack them to continue working by the expedient of manually
dropping the pg_depend entries that this fix installs.
Back-patch into v15. I don't want to risk changing this behavior
in stable branches, but it seems not too late for v15. (Since
we have already forced initdb for beta3, we can be sure that all
production v15 installations will have these added dependencies.)
Discussion: https://postgr.es/m/182492.1658431155@sss.pgh.pa.us
Things like "opt_name" can well be shared by various commands rather
than there being multiple definitions of the same thing. Rename these
productions and move them to appear together in gram.y, which may
improve chances of reuse in the future.
Discussion: https://postgr.es/m/20220721174212.cmitjpuimx6ssyyj@alvherre.pgsql
Commit c6f2f016 added an explicit check for a Windows "junction point".
That turned out to be needed only because get_dirent_type() was busted
on Windows. It's been fixed by commit 9d3444dc, so remove it.
Add a TAP-test to demonstrate that in-place tablespaces are copied by
pg_basebackup. This exercises the codepath that would fail before
c6f2f016 on Windows, and shows that it still doesn't fail now that we're
using get_dirent_type() on both Windows and Unix.
Back-patch to 15, where in-place tablespaces arrived and caused this
problem (ie directories where previously only symlinks were expected).
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CA%2BhUKGLzLK4PUPx0_AwXEWXOYAejU%3D7XpxnYE55Y%2Be7hB2N3FA%40mail.gmail.com
Commit 87e6ed7c8 added code that intended to report Windows "junction
points" as DT_LNK (the same way we report symlinks on Unix). Windows
junction points are *also* directories according to the Windows
attributes API, and we were reporting them as as DT_DIR. Change the
order we check the attribute flags, to prioritize DT_LNK.
If at some point we start using Windows' recently added real symlinks
and need to distinguish them from junction points, we may need to
rethink this, but for now this continues the tradition of wrapper
functions that treat junction points as symlinks.
Back-patch to 14, where get_dirent_type() landed.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/CA%2BhUKGLzLK4PUPx0_AwXEWXOYAejU%3D7XpxnYE55Y%2Be7hB2N3FA%40mail.gmail.com
Discussion: https://postgr.es/m/20220721111751.x7hod2xgrd76xr5c%40alvherre.pgsql
O_FSYNC was a pre-POSIX way of spelling O_SYNC, supported since commit
9d645fd84c for non-conforming operating systems of the time. It's not
needed on any modern system. We can just use standard O_SYNC directly
if it exists (= all targeted systems except Windows), and get rid of our
OPEN_SYNC_FLAG macro.
Similarly for standard O_DSYNC, we can just use that directly if it
exists (= all targeted systems except DragonFlyBSD), and get rid of our
OPEN_DATASYNC_FLAG macro.
We still avoid choosing open_datasync as a default value for
wal_sync_method if O_DSYNC has the same value as O_SYNC (= only
OpenBSD), so there is no change in default behavior.
Discussion: https://postgr.es/m/CA%2BhUKGJE7y92NY7FG2ftUbZUaqohBU65_Ys_7xF5mUHo4wirTQ%40mail.gmail.com
Commit 4f658dc8 provided the traditional BSD fls() function in
src/port/fls.c so it could be used in several places. Later we added a
bunch of similar facilities in pg_bitutils.h, based on compiler
builtins that map to hardware instructions. It's a bit confusing to
have both 1-based and 0-based variants of this operation in use in
different parts of the tree, and neither is blessed by a standard.
Let's drop fls.c and the configure probe, and reuse the newer code.
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CA%2BhUKG%2B7dSX1XF8yFGmYk-%3D48dbjH2kmzZj16XvhbrWP-9BzRg%40mail.gmail.com
This allows users to omit the statistics name in a CREATE STATISTICS
command, letting the system auto-generate a sensible, unique name,
putting the statistics object in the same schema as the table.
Simon Riggs, reviewed by Matthias van de Meent.
Discussion: https://postgr.es/m/CANbhV-FGD2d_C3zFTfT2aRfX_TaPSgOeKES58RLZx5XzQp5NhA@mail.gmail.com
Due to lack of concern for the case in the dependency code, it's
possible to drop a column of a composite type even though stored
queries have references to the dropped column via functions-in-FROM
that return the composite type. There are "soft" references,
namely FROM-clause aliases for such columns, and "hard" references,
that is actual Vars referring to them. The right fix for hard
references is to add dependencies preventing the drop; something
we've known for many years and not done (and this commit still doesn't
address it). A "soft" reference shouldn't prevent a drop though.
We've been around on this before (cf. 9b35ddce9, 2c4debbd0), but
nobody had noticed that the current behavior can result in dump/reload
failures, because ruleutils.c can print more column aliases than the
underlying composite type now has. So we need to rejigger the
column-alias-handling code to treat such columns as dropped and not
print aliases for them.
Rather than writing new code for this, I used expandRTE() which already
knows how to figure out which function result columns are dropped.
I'd initially thought maybe we could use expandRTE() in all cases, but
that fails for EXPLAIN's purposes, because the planner strips a lot of
RTE infrastructure that expandRTE() needs. So this patch just uses it
for unplanned function RTEs and otherwise does things the old way.
If there is a hard reference (Var), then removing the column alias
causes us to fail to print the Var, since there's no longer a name
to print. Failing seems less desirable than printing a made-up
name, so I made it print "?dropped?column?" instead.
Per report from Timo Stolz. Back-patch to all supported branches.
Discussion: https://postgr.es/m/5c91267e-3b6d-5795-189c-d15a55d61dbb@nullachtvierzehn.de
This patch adds a new SUBSCRIPTION parameter "origin". It specifies
whether the subscription will request the publisher to only send changes
that don't have an origin or send changes regardless of origin. Setting it
to "none" means that the subscription will request the publisher to only
send changes that have no origin associated. Setting it to "any" means
that the publisher sends changes regardless of their origin. The default
is "any".
Usage:
CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=postgres port=9999'
PUBLICATION pub1 WITH (origin = none);
This can be used to avoid loops (infinite replication of the same data)
among replication nodes.
This feature allows filtering only the replication data originating from
WAL but for initial sync (initial copy of table data) we don't have such a
facility as we can only distinguish the data based on origin from WAL. As
a follow-up patch, we are planning to forbid the initial sync if the
origin is specified as none and we notice that the publication tables were
also replicated from other publishers to avoid duplicate data or loops.
We forbid to allow creating origin with names 'none' and 'any' to avoid
confusion with the same name options.
Author: Vignesh C, Amit Kapila
Reviewed-By: Peter Smith, Amit Kapila, Dilip Kumar, Shi yu, Ashutosh Bapat, Hayato Kuroda
Discussion: https://postgr.es/m/CALDaNm0gwjY_4HFxvvty01BOT01q_fJLKQ3pWP9=9orqubhjcQ@mail.gmail.com
This renames the relation storing the relfilenode state into something
more generic as it also stores data for non-toast relations. A
restriction on the number of digits used for the OID number when
filtering toast relation names is removed, while on it, as there is no
need for it.
Reported-by: Justin Pryzby
Reviewed-by: Justin Pryzby
Discussion: https://postgr.es/m/20220719022652.GE12702@telsasoft.com
Most of these have been introduced in d2d3547 with the new pattern
validation logic, and would leak memory worth an amount of one
PQExpBuffer each time (as of 256 bytes at minimum, possibly more).
Most of the patch has been written by Tang Haiying, with a few tweaks
coming from Álvaro Herrera.
Reported-by: Tang Haiying
Author: Tang Haiying, Álvaro Herrera
Reviewed-by: Mark Dilger, Andres Freund, Álvaro Herrera, Tom Lane, Japin
Li, Michael Paquier, Junwang Zhao
Backpatch-through: 15
Commit 964d01ae9 marked a lot of fields as read_write_ignore
to stay consistent with what was dumped by the manually-maintained
outfuncs.c code. However, it seems that a pretty fair number
of those omissions were either flat-out oversights, or a shortcut
taken because hand-written code seemed like it'd be too much trouble.
Let's upgrade things where it seems to make sense to dump.
To do this, we need to add support to gen_node_support.pl and
outfuncs.c for variable-length arrays of Node pointers. That's
pretty straightforward given the model of the existing code
for arrays of scalars, but I found I needed to tighten the
type-recognizing regexes in gen_node_support.pl. (As they
stood, they mistook "foo **" for "foo *". Make sure they're
all fully anchored to prevent additional problems.)
The main thing left un-done here is that a lot of partitioning-related
structs are still not dumped, because they are bare structs not Nodes.
I'm not sure about the wisdom of that choice ... but changing it would
be fairly invasive, so it probably requires more justification than
just making planner node dumps more complete.
Discussion: https://postgr.es/m/1295668.1658258637@sss.pgh.pa.us
Without processing shared_preload_libraries, it's impossible to
recover if custom WAL resource managers are needed. It may also pose a
problem running VACUUM on a table with a custom AM, if the module
implementing the AM is expecting to be loaded by
shared_preload_libraries.
The reason this wasn't done before was just the general principle to
do fewer things in single-user mode. But it's easy enough to just set
shared_preload_libraries to empty, for the same effect.
Discussion: https://postgr.es/m/9decc18a42634f8a2f15c97a385a0f51a752f396.camel%40j-davis.com
Reviewed-by: Tom Lane, Andres Freund
Backpatch-through: 15
When the ability to print variable-length-array fields was first
added to outfuncs.c, there was no corresponding read capability,
as it was used only for debug dumps of planner-internal Nodes.
Not a lot of thought seems to have been put into the output format:
it's just the space-separated array elements and nothing else.
Later such fields appeared in Plan nodes, and still later we grew
read support so that Plans could be transferred to parallel workers,
but the original text format wasn't rethought. It seems inadequate
to me because (a) no cross-check is possible that we got the right
number of array entries, (b) we can't tell the difference between
a NULL pointer and a zero-length array, and (c) except for
WRITE_INDEX_ARRAY, we'd crash if a non-zero length is specified
when the pointer is NULL, a situation that can arise in some fields
that we currently conveniently avoid printing.
Since we're currently in a campaign to make the Node infrastructure
generally more it-just-works-without-thinking-about-it, now seems
like a good time to improve this.
Let's adopt a format similar to that used for Lists, that is "<>"
for a NULL pointer or "(item item item)" otherwise. Also retool
the code to not have so many copies of the identical logic.
I bumped catversion out of an abundance of caution, although I think
that we don't use any such array fields in Nodes that can get into
the catalogs.
Discussion: https://postgr.es/m/1528424.1658272135@sss.pgh.pa.us
This allows aliases for sub-SELECTs and VALUES clauses in the FROM
clause to be omitted.
This is an extension of the SQL standard, supported by some other
database systems, and so eases the transition from such systems, as
well as removing the minor inconvenience caused by requiring these
aliases.
Patch by me, reviewed by Tom Lane.
Discussion: https://postgr.es/m/CAEZATCUCGCf82=hxd9N5n6xGHPyYpQnxW8HneeH+uP7yNALkWA@mail.gmail.com
Windows 10 gained support for flushing NTFS files with fdatasync()
semantics. The main advantage over open_datasync (in Windows API terms
FILE_FLAG_WRITE_THROUGH) is that the latter does not flush SATA drive
caches. The default setting is not changed, so users have to opt in to
this.
Discussion: https://postgr.es/m/CA%2BhUKGJZJVO%3DiX%2Beb-PXi2_XS9ZRqnn_4URh0NUQOwt6-_51xQ%40mail.gmail.com
When a non-exclusive backup is canceled, do_pg_abort_backup() is called
and resets some variables set by pg_backup_start (pg_start_backup in v14
or before). But previously it forgot to reset the session state indicating
whether a non-exclusive backup is in progress or not in this session.
This issue could cause an assertion failure when the session running
BASE_BACKUP is terminated after it executed pg_backup_start and
pg_backup_stop (pg_stop_backup in v14 or before). Also it could cause
a segmentation fault when pg_backup_stop is called after BASE_BACKUP
in the same session is canceled.
This commit fixes the issue by making do_pg_abort_backup reset
that session state.
Back-patch to all supported branches.
Author: Fujii Masao
Reviewed-by: Kyotaro Horiguchi, Masahiko Sawada, Michael Paquier, Robert Haas
Discussion: https://postgr.es/m/3374718f-9fbf-a950-6d66-d973e027f44c@oss.nttdata.com
Multiple non-exclusive backups are able to be run conrrently in different
sessions. But, in the same session, only one non-exclusive backup can be
run at the same moment. If pg_backup_start (pg_start_backup in v14 or before)
is called in the middle of another non-exclusive backup in the same session,
an error is thrown.
However, previously, in logical replication walsender mode, even if that
walsender session had already called pg_backup_start and started
a non-exclusive backup, it could execute BASE_BACKUP command and
start another non-exclusive backup. Which caused subsequent pg_backup_stop
to throw an error because BASE_BACKUP unexpectedly reset the session state
marked by pg_backup_start.
This commit prevents BASE_BACKUP command in the middle of another
non-exclusive backup in the same session.
Back-patch to all supported branches.
Author: Fujii Masao
Reviewed-by: Kyotaro Horiguchi, Masahiko Sawada, Michael Paquier, Robert Haas
Discussion: https://postgr.es/m/3374718f-9fbf-a950-6d66-d973e027f44c@oss.nttdata.com
Detail and hint messages should be full sentences and should end with a
period, but some of the messages newly-introduced in v15 did not follow
that.
Author: Justin Pryzby
Reviewed-by: Álvaro Herrera
Discussion: https://postgr.es/m/20220719120948.GF12702@telsasoft.com
Backpatch-through: 15
We allow users to set the values of not-yet-loaded extension GUCs,
remembering those values in "placeholder" GUC entries. When/if
the extension is loaded later in the session, we need to verify that
the user had permissions to set the GUC. That was done correctly
before commit a0ffa885e, but as of that commit, we'd check the
permissions of the active role when the LOAD happens, not the role
that had set the value. (This'd be a security bug if it had made it
into a released version.)
In principle this is simple enough to fix: we just need to remember
the exact role OID that set each GUC value, and use that not
GetUserID() when verifying permissions. Maintaining that data in
the guc.c data structures is slightly tedious, but fortunately it's
all basically just copy-n-paste of the logic for tracking the
GucSource of each setting, as we were already doing.
Another oversight is that validate_option_array_item() hadn't
been taught to check for granted GUC privileges. This appears
to manifest only in that ALTER ROLE/DATABASE RESET ALL will
fail to reset settings that the user should be allowed to reset.
Patch by myself and Nathan Bossart, per report from Nathan Bossart.
Back-patch to v15 where the faulty code came in.
Discussion: https://postgr.es/m/20220706224727.GA2158260@nathanxps13
This is mostly just to get outfuncs.c support for them, so that
the agginfos and aggtransinfos lists can be dumped when dumping
the contents of PlannerInfo.
While here, improve some related comments; notably, clean up
obsolete comments left over from when preprocess_minmax_aggregates
had to make its own scan of the query tree.
Discussion: https://postgr.es/m/742479.1658160504@sss.pgh.pa.us
setrefs.c contains logic to discard no-op SubqueryScan nodes, that is,
ones that have no qual to check and copy the input targetlist unchanged.
(Formally it's not very nice to be applying such optimizations so late
in the planner, but there are practical reasons for it; mostly that we
can't unify relids between the subquery and the parent query until we
flatten the rangetable during setrefs.c.) This behavior falsifies our
previous cost estimates, since we would've charged cpu_tuple_cost per
row just to pass data through the node. Most of the time that's little
enough to not matter, but there are cases where this effect visibly
changes the plan compared to what you would've gotten with no
sub-select.
To improve the situation, make the callers of cost_subqueryscan tell
it whether they think the targetlist is trivial. cost_subqueryscan
already has the qual list, so it can check the other half of the
condition easily. It could make its own determination of tlist
triviality too, but doing so would be repetitive (for callers that
may call it several times) or unnecessarily expensive (for callers
that can determine this more cheaply than a general test would do).
This isn't a 100% solution, because createplan.c also does things
that can falsify any earlier estimate of whether the tlist is
trivial. However, it fixes nearly all cases in practice, if results
for the regression tests are anything to go by.
setrefs.c also contains logic to discard no-op Append and MergeAppend
nodes. We did have knowledge of that behavior at costing time, but
somebody failed to update it when a check on parallel-awareness was
added to the setrefs.c logic. Fix that while we're here.
These changes result in two minor changes in query plans shown in
our regression tests. Neither is relevant to the purposes of its
test case AFAICT.
Patch by me; thanks to Richard Guo for review.
Discussion: https://postgr.es/m/2581077.1651703520@sss.pgh.pa.us
Per discussion, this commit includes a couple of changes to these two
flavors of REINDEX:
* The grammar is changed to make the name of the object optional, hence
one can rebuild all the indexes of the wanted area by specifying only
"REINDEX DATABASE;" or "REINDEX SYSTEM;". Previously, the object name
was mandatory and had to match the name of the database on which the
command is issued.
* REINDEX DATABASE is changed to ignore catalogs, making this task only
possible with REINDEX SYSTEM. This is a historical change, but there
was no way to work only on the indexes of a database without touching
the catalogs. We have discussed more approaches here, like the addition
of an option to skip the catalogs without changing the original
behavior, but concluded that what we have here is for the best.
This builds on top of the TAP tests introduced in 5fb5b6c, showing the
change in behavior for REINDEX SYSTEM. reindexdb is updated so as we do
not issue an extra REINDEX SYSTEM when working on a database in the
non-concurrent case, something that was confusing when --concurrently
got introduced, so this simplifies the code.
Author: Simon Riggs
Reviewed-by: Ashutosh Bapat, Bernd Helmle, Álvaro Herrera, Cary Huang,
Michael Paquier
Discussion: https://postgr.es/m/CANbhV-H=NH6Om4-X6cRjDWfH_Mu1usqwkuYVp-hwdB_PSHWRfg@mail.gmail.com
Adding such commands in the main regression test suite is not a good
approach performance-wise as it impacts all the objects in the
regression database, so this additional coverage is added in the TAP
tests of reindexdb where we already run a few REINDEX commands with
SYSTEM and DATABASE so there is no runtime difference for the test.
This additional coverage checks which relations are rewritten with
relfilenode changes, as of:
- a toast index in user table.
- a toast index in catalog table.
- a catalog index.
- a user index.
This test suite is something I have implemented for a separate patch
that reworks a bit the way we handle these two REINDEX behaviors, but it
has enough value in itself to be in a separate commit. This also makes
easier to follow what actually changes once the REINDEX logic is
reworked (currently, DABATASE rewrites both catalog and user tables, and
SYSTEM works only on catalogs).
Discussion: https://postgr.es/m/YtOqA7ldcJQADEE8@paquier.xyz
This fixes problems on windows when logging collector is used in a service,
failing with:
FATAL: could not redirect stderr: Bad file descriptor
This is triggered by 76e38b37a5. The problem is that STDOUT/STDERR_FILENO
aren't defined on windows, which lead us to use _fileno(stdout) etc, but that
doesn't work if stdout/stderr are closed.
Author: Andres Freund <andres@anarazel.de>
Reported-By: Sandeep Thakkar <sandeep.thakkar@enterprisedb.com>
Message-Id: 20220520164558.ozb7lm6unakqzezi@alap3.anarazel.de (on pgsql-packagers)
Backpatch: 15-, where 76e38b37a5 came in
Because they are not available we've used _fileno(stdin) in some places, but
that doesn't reliably work, because stdin might be closed. This is the
prerequisite of the subsequent commit, fixing a failure introduced in
76e38b37a5.
Author: Andres Freund <andres@anarazel.de>
Reported-By: Sandeep Thakkar <sandeep.thakkar@enterprisedb.com>
Message-Id: 20220520164558.ozb7lm6unakqzezi@alap3.anarazel.de (on pgsql-packagers)
Backpatch: 15-, where 76e38b37a5 came in
parse.pl and check_rules.pl used "no warnings 'uninitialized'",
which doesn't seem like it measures up to current project standards.
Removing that shows that it was hiding various places that accessed
off the end of an array, which are easily protected by minor logic
adjustments. There's no change in the script results.
While here, improve the Makefile rule that invokes these scripts.
It neglected to depend on check_rules.pl, so that editing that file
didn't result in re-running the check; and it ran check_rules.pl
after building preproc.y, so that if check_rules.pl did fail the
next "make" attempt would just bypass it. check_rules.pl failures
are sufficiently un-heard-of that I don't feel a need to back-patch
this.
Discussion: https://postgr.es/m/838180.1658181982@sss.pgh.pa.us
In db0a272d12 I used open(our $something, ...), which perlcritic doesn't
like. It looks like the warning is due to perlcritic knowing about 'my' but
not 'our' when checking for bareword file handles.
However, it's clearly unnecessary to use "our" here, change it to "my".
Via buildfarm member crake and discussion with Tom Lane.
Discussion: https://postgr.es/m/20220718215042.sl3hivoupdb7lkwv@awork3.anarazel.de
This is in preparation for building postgres with meson / ninja.
Move the dtrace postprocessing sed commands into a separate file so
that it can be shared by meson. Also split the rule into two for
proper dependency declaration.
Reviewed-by: Andres Freund <andres@anarazel.de>
Author: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://postgr.es/m/5e216522-ba3c-f0e6-7f97-5276d0270029@enterprisedb.com
This is in preparation for building postgres with meson / ninja.
When building with meson, commands are run at the root of the build tree. Add
an option to put build output into the appropriate place. This can be utilized
by src/tools/msvc/ for a minor simplification, which also provides some
coverage for the new option.
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/5e216522-ba3c-f0e6-7f97-5276d0270029@enterprisedb.com
This is in preparation for building postgres with meson / ninja.
When building with meson, commands are run at the root of the build tree. Add
an option to put build output into the appropriate place.
Author: Andres Freund <andres@anarazel.de>
Author: Peter Eisentraut <peter@eisentraut.org>
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Discussion: https://postgr.es/m/5e216522-ba3c-f0e6-7f97-5276d0270029@enterprisedb.com
This is in preparation for building postgres with meson / ninja.
meson's 'capture' (redirecting stdout to a file) is a bit slower than programs
redirecting output themselves (mostly due to a python wrapper necessary for
windows). That doesn't matter for most things, but errcodes.h is a dependency
of nearly everything, making it a bit faster seem worthwhile.
Medium term it might also be worth avoiding writing errcodes.h if its contents
didn't actually change, to avoid unnecessary recompilations.
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/5e216522-ba3c-f0e6-7f97-5276d0270029@enterprisedb.com
This is in preparation for building postgres with meson / ninja.
When building with meson, commands are run at the root of the build tree. Add
an option to put build output into the appropriate place. This can be utilized
by src/tools/msvc/ for a minor simplification, which also provides some
coverage for the new option.
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/5e216522-ba3c-f0e6-7f97-5276d0270029@enterprisedb.com
This is in preparation for building postgres with meson / ninja.
We already have duplicated code for this between the make and msvc
builds. Adding a third copy seems like a bad plan, thus move the generation
into a perl script.
As we don't want to rely on perl being available for builds from tarballs,
generate the file during distprep.
Author: Peter Eisentraut <peter@eisentraut.org>
Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/5e216522-ba3c-f0e6-7f97-5276d0270029@enterprisedb.com
This is in preparation for building postgres with meson / ninja.
When building with meson, commands are run at the root of the build tree. Add
an option to put build output into the appropriate place. This can be utilized
by src/tools/msvc/ for a minor simplification, which also provides some
coverage for the new option.
Add option to generate a timestamp for check_rules.pl, so that proper
dependencies on it having been run can be generated.
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/5e216522-ba3c-f0e6-7f97-5276d0270029@enterprisedb.com
This is in preparation for building postgres with meson / ninja.
When building with meson, commands are run at the root of the build tree. Add
an option to put build output into the appropriate place. This can be utilized
by src/tools/msvc/ for a minor simplification, which also provides some
coverage for the new option.
To deal with dependencies to the variable set of input files to this script,
add an option to generate a dependency file (which meson / ninja can consume).
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/5e216522-ba3c-f0e6-7f97-5276d0270029@enterprisedb.com
Commit e3fcca0d0d reverted modifications to HOT for BRIN, but it also
removed a couple unrelated tests from stats.sql. Reinstate those tests.
Reported-by: Peter Eisentraut
A code comment said that the standard does not define a number for
ERRCODE_SQL_JSON_ITEM_CANNOT_BE_CAST_TO_TARGET_TYPE, but this was
fixed in a later draft version of the standard, so use that number
now.
This is in preparation for defaulting to -fvisibility=hidden in extensions,
instead of relying on all symbols in extensions to be exported.
This should have been committed before 089480c077, but something in my commit
scripts went wrong.
Author: Andres Freund <andres@anarazel.de>
Reviewed-By: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/20211101020311.av6hphdl6xbjbuif@alap3.anarazel.de
Until now postgres built extension libraries with global visibility, i.e.
exporting all symbols. On the one platform where that behavior is not
natively available, namely windows, we emulate it by analyzing the input files
to the shared library and exporting all the symbols therein.
Not exporting all symbols is actually desirable, as it can improve loading
speed, reduces the likelihood of symbol conflicts and can improve intra
extension library function call performance. It also makes the non-windows
builds more similar to windows builds.
Additionally, with meson implementing the export-all-symbols behavior for
windows, turns out to be more verbose than desirable.
This patch adds support for hiding symbols by default and, to counteract that,
explicit symbol visibility annotation for compilers that support
__attribute__((visibility("default"))) and -fvisibility=hidden. That is
expected to be most, if not all, compilers except msvc (for which we already
support explicit symbol export annotations).
Now that extension library symbols are explicitly exported, we don't need to
export all symbols on windows anymore, hence remove that behavior from
src/tools/msvc. The supporting code can't be removed, as we still need to
export all symbols from the main postgres binary.
Author: Andres Freund <andres@anarazel.de>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/20211101020311.av6hphdl6xbjbuif@alap3.anarazel.de
This is in preparation for defaulting to -fvisibility=hidden in extensions,
instead of exporting all symbols. For that symbols intended to be exported
need to be tagged with PGDLLEXPORT. Most extensions only need to do so for
_PG_init() and functions defined with PG_FUNCTION_INFO_V1. Adding central
declarations avoids each extension having to add PGDLLEXPORT. Any existing
declarations in extensions will continue to work if fmgr.h is included before
them, otherwise compilation for Windows will fail.
Author: Andres Freund <andres@anarazel.de>
Reviewed-By: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/20211101020311.av6hphdl6xbjbuif@alap3.anarazel.de
The patch that added regcollation doesn't seem to have been too
thorough about supporting it everywhere that other reg* types
are supported. Fix that. (The find_expr_references omission
is moderately serious, since it could result in missing expression
dependencies. The others are less exciting.)
Noted while fixing bug #17483. Back-patch to v13 where
regcollation was added.
Discussion: https://postgr.es/m/1423433.1652722406@sss.pgh.pa.us
Some of the test cases added by commit 3a0e38504 are failing
intermittently in CI testing. It looks like, when a connection
attempt fails, it's possible for psql to exit and the test script
to slurp up the postmaster's log file before the connected backend
has managed to write the log entry we're expecting to see.
It's not clear whether that's fixable in any robust way. Pending
more thought, just comment out the log_like checks. The ones in
connect_ok tests should be fine, since surely the log entry should
be emitted before we complete the client auth sequence. I took
out all the ones in connect_fails tests though.
Discussion: https://postgr.es/m/E1oCNLk-000LCH-Af@gemulon.postgresql.org
reset_shared just invokes CreateSharedMemoryAndSemaphores, so let's
get rid of it and invoke that directly. This removes a confusing
seeming-inconsistency between the postmaster's startup sequence
and the startup sequence used in standalone mode.
Nathan Bossart, reviewed by Pavel Borisov
Discussion: https://postgr.es/m/20220329221702.GA559657@nathanxps13
This replaces all MemSet() calls with struct initialization where that
is easily and obviously possible. (For example, some cases have to
worry about padding bits, so I left those.)
(The same could be done with appropriate memset() calls, but this
patch is part of an effort to phase out MemSet(), so it doesn't touch
memset() calls.)
Reviewed-by: Ranier Vilela <ranier.vf@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://www.postgresql.org/message-id/9847b13c-b785-f4e2-75c3-12ec77a3b05c@enterprisedb.com
Since commit a65e0864, we've required Unix systems to have
sigprocmask(). As noted in that commit's message, we were still
emulating the historical pre-standard sigsetmask() function in our
Windows support code. Emulate standard sigprocmask() instead, for
consistency.
The PG_SETMASK() abstraction is now redundant and all calls could in
theory be replaced by plain sigprocmask() calls, but that isn't done by
this commit.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/3153247.1657834482%40sss.pgh.pa.us
Commit 4518c798 blocks signals for a short region of code, but it
assumed that whatever called it had the signal mask set to UnBlockSig on
entry. That may be true today (or may even not be, in extensions in the
wild), but it would be better not to make that assumption. We should
save-and-restore the caller's signal mask.
The PG_SETMASK() portability macro couldn't be used for that, which is
why it wasn't done before. But... considering that commit a65e0864
established back in 9.6 that supported POSIX systems have sigprocmask(),
and that this is POSIX-only code, there is no reason not to use standard
sigprocmask() directly to achieve that.
Back-patch to all supported releases, like 4518c798 and 80845b7c.
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CA%2BhUKGKx6Biq7_UuV0kn9DW%2B8QWcpJC1qwhizdtD9tN-fn0H0g%40mail.gmail.com
Currently, debugging client certificate verification failures is
mostly limited to looking at the TLS alert code on the client side.
For simple deployments, sometimes it's enough to see "sslv3 alert
certificate revoked" and know exactly what needs to be fixed, but if
you add any more complexity (multiple CA layers, misconfigured CA
certificates, etc.), trying to debug what happened based on the TLS
alert alone can be an exercise in frustration.
Luckily, the server has more information about exactly what failed in
the chain, and we already have the requisite callback implemented as a
stub. We fill that in, collect the data, and pass the constructed
error message back to the main code via a static variable. This lets
us add our error details directly to the final "could not accept SSL
connection" log message, as opposed to issuing intermediate LOGs.
It ends up looking like
LOG: connection received: host=localhost port=43112
LOG: could not accept SSL connection: certificate verify failed
DETAIL: Client certificate verification failed at depth 1: unable to get local issuer certificate.
Failed certificate data (unverified): subject "/CN=Test CA for PostgreSQL SSL regression test client certs", serial number 2315134995201656577, issuer "/CN=Test root CA for PostgreSQL SSL regression test suite".
The length of the Subject and Issuer strings is limited to prevent
malicious client certs from spamming the logs. In case the truncation
makes things ambiguous, the certificate's serial number is also
logged.
Author: Jacob Champion <pchampion@vmware.com>
Discussion: https://www.postgresql.org/message-id/flat/d13c4a5787c2a3f83705124f0391e0738c796751.camel@vmware.com
For some systems, we need to avoid unsatisfied-external-reference
errors in static inlines. See
27d2693187 for example. In order to
test that on other systems, the gcc option -fkeep-inline-functions can
be used. But it actually is a bit stricter than what we currently
have in place, so fix up a few more places along the lines of the
above commit. (This undoes part of commit
2cd2569c72b8920048e35c31c9be30a6170e1410.)
(Note, this does not add that gcc option anywhere to the build system,
it just makes it possible to use it successfully manually.)
Discussion: https://www.postgresql.org/message-id/flat/E1oBgIW-002ehP-VJ%40gemulon.postgresql.org
Noticed while working in this area. This code was introduced in PG15,
which is still in beta, so backpatch to there for consistency.
Backpatch-through: 15
Teach this script to handle function pointer fields honestly.
Previously they were just silently ignored, but that's not likely to
be a behavior we can accept indefinitely. This mostly entails fixing
it so that a field declaration spanning multiple lines can be parsed,
because we have a bunch of such fields that're laid out that way.
But that's a good improvement in its own right.
With that change and a minor regex adjustment, the only struct it
fails to parse in the node-defining headers is A_Const, because
of the embedded union. The path of least resistance is to move
that union declaration outside the struct.
Having done those things, we can make it error out if it finds
any within-struct syntax it doesn't understand, which seems like
a pretty important property for robustness.
This commit doesn't change the output files at all; it's just in
the way of future-proofing.
Discussion: https://postgr.es/m/2593369.1657759779@sss.pgh.pa.us
Previously we displayed "DSMFillZeroWrite" while in posix_fallocate(),
because we shared the same wait event for "mmap" and "posix" DSM types.
Let's introduce a new wait event "DSMAllocate", to be more accurate.
Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20220711174518.yldckniicknsxgzl%40awork3.anarazel.de
In early releases of the DSM infrastructure, it was possible to resize
segments. That was removed in release 12 by commit 3c60d0fa. Now the
ftruncate() + posix_fallocate() sequence during DSM segment creation has
a redundant step: we're always extending from zero to the desired size,
so we might as well just call posix_fallocate().
Let's also include the remaining ftruncate() call (non-Linux POSIX
systems) in the wait event reporting, for good measure.
Discussion: https://postgr.es/m/CA%2BhUKGJSm-nq8s%2B_59zb7NbFQF-OS%3DxTnTAiGLrQpuSmU2y_1A%40mail.gmail.com
On Linux, we call posix_fallocate() on shm_open()'d memory to avoid
later potential SIGBUS (see commit 899bd785).
Based on field reports of systems stuck in an EINTR retry loop there,
there, we made it possible to break out of that loop via slightly odd
coding where the CHECK_FOR_INTERRUPTS() call was somewhat removed from
the loop (see commit 422952ee).
On further reflection, that was not a great choice for at least two
reasons:
1. If interrupts were held, the CHECK_FOR_INTERRUPTS() would do nothing
and the EINTR error would be surfaced to the user.
2. If EINTR was reported but neither QueryCancelPending nor
ProcDiePending was set, then we'd dutifully retry, but with a bit more
understanding of how posix_fallocate() works, it's now clear that you
can get into a loop that never terminates. posix_fallocate() is not a
function that can do some of the job and tell you about progress if it's
interrupted, it has to undo what it's done so far and report EINTR, and
if signals keep arriving faster than it can complete (cf recovery
conflict signals), you're stuck.
Therefore, for now, we'll simply block most signals to guarantee
progress. SIGQUIT is not blocked (see InitPostmasterChild()), because
its expected handler doesn't return, and unblockable signals like
SIGCONT are not expected to arrive at a high rate. For good measure,
we'll include the ftruncate() call in the blocked region, and add a
retry loop.
Back-patch to all supported releases.
Reported-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Reported-by: Nicola Contu <nicola.contu@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20220701154105.jjfutmngoedgiad3%40alvherre.pgsql
Thomas Munro
Robert Haas
Tom Lane
Daniel Gustafsson
John Naylor
Michael Paquier
Amit Kapila
David Rowley
Andrew Dunstan
Alvaro Herrera
Fujii Masao
Alexander Korotkov
Heikki Linnakangas
Dean Rasheed
Jeff Davis
Peter Eisentraut
Andres Freund
Tomas Vondra