The problem report was about setting GUCs in the startup packet for a
physical replication connection. Setting the GUC required an ACL
check, which performed a lookup on pg_parameter_acl.parname. The
catalog cache was hardwired to use DEFAULT_COLLATION_OID for
texteqfast() and texthashfast(), but the database default collation
was uninitialized because it's a physical walsender and never connects
to a database. In versions 18 and later, this resulted in a NULL
pointer dereference, while in version 17 it resulted in an ERROR.
As the comments stated, using DEFAULT_COLLATION_OID was arbitrary
anyway: if the collation actually mattered, it should have used the
column's actual collation. (In the catalog, some text columns are the
default collation and some are "C".)
Fix by using C_COLLATION_OID, which doesn't require any initialization
and is always available. When any deterministic collation will do,
it's best to consistently use the simplest and fastest one, so this is
a good idea anyway.
Another problem was raised in the thread, which this commit doesn't
fix (see second discussion link).
Reported-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/D18AD72A-5004-4EF8-AF80-10732AF677FA@yandex-team.ru
Discussion: https://postgr.es/m/4524ed61a015d3496fc008644dcb999bb31916a7.camel%40j-davis.com
Backpatch-through: 17
Commit 7a1f0f8747 optimized the slot verification query but
overlooked cases where all logical replication slots are already
invalidated. In this scenario, the CTE returns no rows, causing the
main query (which used a cross join) to return an empty result even
when invalid slots exist.
This commit fixes this by using a LEFT JOIN with the CTE, ensuring
that slots are properly reported even if the CTE returns no rows.
Author: Lakshmi N <lakshmin.jhs@gmail.com>
Reviewed-by: Shveta Malik <shveta.malik@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CA+3i_M8eT6j8_cBHkYykV-SXCxbmAxpVSKptjDVq+MFtpT-Paw@mail.gmail.com
Make sure that function declarations use names that exactly match the
corresponding names from function definitions in a few places. Most of
these inconsistencies were introduced during Postgres 19 development.
This commit was written with help from clang-tidy, by mechanically
applying the same rules as similar clean-up commits (the earliest such
commit was commit 035ce1fe).
to_char() allocates its output buffer with 8 bytes per formatting
code in the pattern. If the locale's currency symbol, thousands
separator, or decimal or sign symbol is more than 8 bytes long,
in principle we could overrun the output buffer. No such locales
exist in the real world, so it seems sufficient to truncate the
symbol if we do see it's too long.
Reported-by: Xint Code
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/638232.1776790821@sss.pgh.pa.us
Backpatch-through: 14
parse_affentry() and addCompoundAffixFlagValue() each collect fields
from an affix file into working buffers of size BUFSIZ. They failed
to defend against overlength fields, so that a malicious affix file
could cause a stack smash. BUFSIZ (typically 8K) is certainly way
longer than any reasonable affix field, but let's fix this while
we're closing holes in this area.
I chose to do this by silently truncating the input before it can
overrun the buffer, using logic comparable to the existing logic in
get_nextfield(). Certainly there's at least as good an argument for
raising an error, but for now let's follow the existing precedent.
Reported-by: Igor Stepansky <igor.stepansky@orca.security>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/864123.1776810909@sss.pgh.pa.us
Backpatch-through: 14
This function writes into a caller-supplied buffer of length
2 * MAXNORMLEN, which should be plenty in real-world cases.
However a malicious affix file could supply an affix long
enough to overrun that. Defend by just rejecting the match
if it would overrun the buffer. I also inserted a check of
the input word length against Affix->replen, just to be sure
we won't index off the buffer, though it would be caller error
for that not to be true.
Also make the actual copying steps a bit more readable, and remove
an unnecessary requirement for the whole input word to fit into the
output buffer (even though it always will with the current caller).
The lack of documentation in this code makes my head hurt, so
I also reverse-engineered a basic header comment for CheckAffix.
Reported-by: Xint Code
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/641711.1776792744@sss.pgh.pa.us
Backpatch-through: 14
When using ALTER TABLE ... MERGE PARTITIONS or ALTER TABLE ... SPLIT
PARTITION, extension dependencies on partition indexes were being lost.
This happened because the new partition indexes are created fresh from
the parent partitioned table's indexes, while the old partition indexes
(with their extension dependencies) are dropped.
Fix this by collecting extension dependencies from source partition
indexes before detaching them, then applying those dependencies to the
corresponding new partition indexes after they're created. The mapping
between old and new indexes is done via their common parent partitioned
index.
For MERGE operations, all source partition indexes sharing a parent
partitioned index must have the same extension dependencies; if they
differ, an error naming both conflicting partition indexes is raised.
The check is implemented by collecting one entry per partition index,
sorting by parent index OID, and comparing adjacent entries in a single
pass. This is order-independent: the same set of partitions produces
the same decision regardless of the order they are listed in the MERGE
command, and subset mismatches are caught in both directions.
For SPLIT operations, the new partition indexes simply inherit all
extension dependencies from the source partition's index.
The regression tests exercising this feature live under
src/test/modules/test_extensions, where the test_ext3 and test_ext5
extensions are available; core regression tests cannot assume any
particular extension is installed.
Author: Matheus Alcantara <matheusssilv97@gmail.com>
Co-authored-by: Alexander Korotkov <aekorotkov@gmail.com>
Reported-by: Kirill Reshke <reshkekirill@gmail.com>
Reviewed-by: Dmitry Koval <d.koval@postgrespro.ru>
Discussion: https://www.postgresql.org/message-id/CALdSSPjXtzGM7Uk4fWRwRMXcCczge5uNirPQcYCHKPAWPkp9iQ%40mail.gmail.com
Formerly, attempting to use WHERE CURRENT OF to update or delete from
a table with virtual generated columns would fail with the error
"WHERE CURRENT OF on a view is not implemented".
The reason was that the check preventing WHERE CURRENT OF from being
used on a view was in replace_rte_variables_mutator(), which presumed
that the only way it could get there was as part of rewriting a query
on a view. That is no longer the case, since replace_rte_variables()
is now also used to expand the virtual generated columns of a table.
Fix by doing the check for WHERE CURRENT OF on a view at parse time.
This is safe, since it is no longer possible for the relkind to change
after the query is parsed (as of b23cd185f).
Reported-by: Satyanarayana Narlapuram <satyanarlapuram@gmail.com>
Author: Satyanarayana Narlapuram <satyanarlapuram@gmail.com>
Author: Dean Rasheed <dean.a.rasheed@gmail.com>
Discussion: https://postgr.es/m/CAHg+QDc_TwzSgb=B_QgNLt3mvZdmRK23rLb+RkanSQkDF40GjA@mail.gmail.com
Backpatch-through: 18
If the SET or WHERE clause of an INSERT ... ON CONFLICT command
references EXCLUDED.col, where col is a virtual generated column, the
column was not properly expanded, leading to an "unexpected virtual
generated column reference" error, or incorrect results.
The problem was that expand_virtual_generated_columns() would expand
virtual generated columns in both the SET and WHERE clauses and in the
targetlist of the EXCLUDED pseudo-relation (exclRelTlist). Then
fix_join_expr() from set_plan_refs() would turn the expanded
expressions in the SET and WHERE clauses back into Vars, because they
would be found to match the expression entries in the indexed tlist
produced from exclRelTlist.
To fix this, arrange for expand_virtual_generated_columns() to not
expand virtual generated columns in exclRelTlist. This forces
set_plan_refs() to resolve generation expressions in the query using
non-virtual columns, as required by the executor.
In addition, exclRelTlist now always contains only Vars. That was
something already claimed in a couple of existing comments in the
planner, which relied on that fact to skip some processing, though
those did not appear to constitute active bugs.
Reported-by: Satyanarayana Narlapuram <satyanarlapuram@gmail.com>
Author: Satyanarayana Narlapuram <satyanarlapuram@gmail.com>
Author: Dean Rasheed <dean.a.rasheed@gmail.com>
Discussion: https://postgr.es/m/CAHg+QDf7wTLz_vqb1wi1EJ_4Uh+Vxm75+b4c-Ky=6P+yOAHjbQ@mail.gmail.com
Backpatch-through: 18
The ri_FetchConstraintInfo() and ri_LoadConstraintInfo() functions
were declared to return const RI_ConstraintInfo *, but callers
sometimes need to modify the struct, requiring casts to drop the
const. Remove the misapplied const qualifiers and the casts that
worked around them.
Reported-by: Peter Eisentraut <peter@eisentraut.org>
Author: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://postgr.es/m/548600ed-8bbb-4e50-8fc3-65091b122276@eisentraut.org
This commit tweaks ALTER INDEX .. ATTACH PARTITION to attempt a
validation of a parent index in the case where an index is already
attached but the parent is not yet valid. This occurs in cases where a
parent index was created invalid such as with CREATE INDEX ONLY, but was
left invalid after an invalid child index was attached (partitioned
indexes set indisvalid to false if at least one partition is
!indisvalid, indisvalid is true in a partitioned table iff all
partitions are indisvalid). This could leave a partition tree in a
situation where a user could not bring the parent index back to valid
after fixing the child index, as there is no built-in mechanism to do
so. This commit relies on the fact that repeated ATTACH PARTITION
commands on the same index silently succeed.
An invalid parent index is more than just a passive issue. It causes
for example ON CONFLICT on a partitioned table if the invalid parent
index is used to enforce a unique constraint.
Some test cases are added to track some of problematic patterns, using a
set of partition trees with combinations of invalid indexes and ATTACH
PARTITION.
Reported-by: Mohamed Ali <moali.pg@gmail.com>
Author: Sami Imseih <sanmimseih@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Haibo Yan <tristan.yim@gmail.com>
Discussion: http://postgr.es/m/CAGnOmWqi1D9ycBgUeOGf6mOCd2Dcf=6sKhbf4sHLs5xAcKVCMQ@mail.gmail.com
Backpatch-through: 14
This neglected to set TAP_TESTS = 1, and partially compensated
for that by writing duplicative hand-made rules for check and
installcheck. That's not really sufficient though. The way
I noticed the error was that "make distclean" didn't clean out
the tmp_check subdirectory, and there might be other consequences.
Do it the standard way instead.
FlushUnlockedBuffer() accepted io_object and io_context arguments but
hardcoded IOOBJECT_RELATION and IOCONTEXT_NORMAL when calling
FlushBuffer(). Pass them through instead. Also fix FlushBuffer() to use
its io_object parameter for I/O timing stats rather than hardcoding
IOOBJECT_RELATION.
Not actively broken since all current callers pass IOOBJECT_RELATION and
IOCONTEXT_NORMAL, so not backpatched.
Author: Chao Li <lic@highgo.com>
Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/BC97546F-5C15-42F2-AD57-CFACDB9657D0@gmail.com
Since b46e1e54d0 allowed setting the VM on-access and 378a21618 set
pd_prune_xid on INSERT, the testing of generic/custom plans in
src/test/regress/sql/plancache.sql was destabilized.
One of the queries of test_mode could have set the pages all-visible and
if autovacuum/autoanalyze ran and updated pg_class.relallvisible, it
would affect whether we got an index-only or sequential scan.
Preclude this by disabling autovacuum and autoanalyze for test_mode and
carefully sequencing when ANALYZE is run.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/71277259-264e-4983-a201-938b404049d7%40gmail.com
GetLocalPinLimit() and GetAdditionalLocalPinLimit(), currently in use
only by the read stream, previously allowed a backend to pin all
num_temp_buffers local buffers. This meant that the read stream could
use every available local buffer for read-ahead, leaving none for other
concurrent pin-holders like other read streams and related buffers like
the visibility map buffer needed during on-access pruning.
This became more noticeable since b46e1e54d0, which allows on-access
pruning to set the visibility map, which meant that some scans also
needed to pin a page of the VM. It caused a test in
src/test/regress/sql/temp.sql to fail in some cases.
Cap the local pin limit to num_temp_buffers / 4, providing some
headroom. This doesn't guarantee that all needed pins will be available
— for example, a backend can still open more cursors than there are
buffers — but it makes it less likely that read-ahead will exhaust the
pool.
Note that these functions are not limited by definition to use in the
read stream; however, this cap should be appropriate in other contexts.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/97529f5a-ec10-46b1-ab50-4653126c6889%40gmail.com
We installed this in commit eea9fa9b2 to protect against foreseeable
mistakes that would break ABI in stable branches by renumbering
NodeTag enum entries. However, we now have much more thorough
ABI stability checks thanks to buildfarm members using libabigail
(see the .abi-compliance-history mechanism). So this incomplete,
single-purpose check seems like an anachronism. I wouldn't object
to keeping it were it not that it requires an additional manual step
when making a new stable git branch. That seems like something easy
to screw up, so let's get rid of it.
This patch just removes the logic that checks for changes in the last
auto-assigned NodeTag value. We still need eea9fa9b2's cross-check
on the supplied list of header files, to prevent divergence between
the makefile and meson build systems. We'll also sometimes need the
nodetag_number() infrastructure for hand-assigning new NodeTags in
stable branches.
Discussion: https://postgr.es/m/1458883.1776143073@sss.pgh.pa.us
We were using "select count(*) into x from generate_series(1,
1_000_000_000_000)" to waste one second waiting for a statement
timeout trap. Aside from consuming CPU to little purpose, this could
easily eat several hundred MB of temporary file space, which has been
observed to cause out-of-disk-space errors in the buildfarm.
Let's just use "pg_sleep(10)", which is far less resource-intensive.
Also update the "when others" exception handler so that if it does
ever again trap an error, it will tell us what error. The cause of
these intermittent buildfarm failures had been obscure for awhile.
Discussion: https://postgr.es/m/557992.1776779694@sss.pgh.pa.us
Backpatch-through: 14
This batch is similar to 462fe0ff62 and addresses a variety of code
style issues, including grammar mistakes, typos, inconsistent variable
names in function declarations, and incorrect function names in comments
and documentation. These fixes have accumulated on the community
mailing lists since the commit mentioned above.
Notably, Alexander Lakhin previously submitted a patch identifying many
of the trivial typos and grammar issues that had been reported on
pgsql-hackers. His patch covered a somewhat large portion of the issues
addressed here, though not all of them.
The documentation changes only affect HEAD.
When a rule action or rule qualification references NEW.col where col
is a generated column (stored or virtual), the rewriter produces
incorrect results.
rewriteTargetListIU removes generated columns from the query's target
list, since stored generated columns are recomputed by the executor
and virtual ones store nothing. However, ReplaceVarsFromTargetList
then cannot find these columns when resolving NEW references during
rule rewriting. For UPDATE, the REPLACEVARS_CHANGE_VARNO fallback
redirects NEW.col to the original target relation, making it read the
pre-update value (same as OLD.col). For INSERT,
REPLACEVARS_SUBSTITUTE_NULL replaces it with NULL. Both are wrong
when the generated column depends on columns being modified.
Fix by building target list entries for generated columns from their
generation expressions, pre-resolving the NEW.attribute references
within those expressions against the query's targetlist, and passing
them together with the query's targetlist to ReplaceVarsFromTargetList.
Back-patch to all supported branches. Virtual generated columns were
added in v18, so the back-patches in pre-v18 branches only handle
stored generated columns.
Reported-by: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Author: Dean Rasheed <dean.a.rasheed@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAHg+QDexGTmCZzx=73gXkY2ZADS6LRhpnU+-8Y_QmrdTS6yUhA@mail.gmail.com
Backpatch-through: 14
When the startup process exists with a FATAL error during PM_STARTUP,
the postmaster called ExitPostmaster() directly, assuming that no other
processes are running at this stage. Since 7ff23c6d27, this
assumption is not true, as the checkpointer, the background writer, the
IO workers and bgworkers kicking in early would be around.
This commit removes the startup-specific shortcut happening in
process_pm_child_exit() for a failing startup process during PM_STARTUP,
falling down to the existing exit() flow to signal all the started
children with SIGQUIT, so as we have no risk of creating orphaned
processes.
This required an extra change in HandleFatalError() for v18 and newer
versions, as an assertion could be triggered for PM_STARTUP. It is now
incorrect. In v17 and older versions, HandleChildCrash() needs to be
changed to handle PM_STARTUP so as children can be waited on.
While on it, fix a comment at the top of postmaster.c. It was claiming
that the checkpointer and the background writer were started after
PM_RECOVERY. That is not the case.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWVoD3V9yhhqSae1_wqcnTdpFY-hDT7dPm5005ZFsL_bpA@mail.gmail.com
Backpatch-through: 15
Replace the outdated DatumGetCString(DirectFunctionCall1(textout, ...))
pattern with TextDatumGetCString(). The macro is the modern, more
efficient way to convert a text Datum to a C string as it avoids
unnecessary function call machinery and handles detoasting internally.
Since plsample serves as reference code for extension authors, it
should follow current idiomatic practices.
Author: Amul Sul <sulamul@gmail.com>
Discussion: https://postgr.es/m/CAAJ_b95-xMvUN1PEqxv8y6g-A-8k+fSgyv20kSZc9eF1wZAUPg@mail.gmail.com
Commit cfcd57111 et al fell over under Valgrind testing.
(It seems to be enough to #define USE_VALGRIND, you don't actually
need to run it under Valgrind to see failures.) The cause is that
remove_rel_from_eclass updates each EquivalenceMember's em_relids,
and those can be aliases of the left_relids or right_relids of some
RestrictInfo in ec_sources. If the update made em_relids empty then
bms_del_member will have pfree'd the relid set, so that the subsequent
attempt to clean up ec_sources accesses already-freed memory.
We missed seeing ill effects before cfcd57111 because (a) if the
pfree happens then we will remove the EquivalenceMember altogether,
making the source RestrictInfo no longer of use, and (b) the
cleanup of ec_sources didn't touch left/right_relids before that.
I'm unclear though on how cfcd57111 managed to pass non-USE_VALGRIND
testing. Apparently we managed to store another Bitmapset into the
freed space before trying to access it, but you'd not think that would
happen 100% of the time. I think what USE_VALGRIND changes is that it
makes list.c much more memory-hungry, so that the freed space gets
claimed by some List node before a Bitmapset can be put there.
This failure can be seen in v16, v17, and master, but oddly enough not
v18. That's because the SJE patch replaced the simple bms_del_members
calls used here with adjust_relid_set, which is careful not to
scribble on its input. But commit 20efbdffe just recently put back
the old coding and thus resurrected the problem.
Discussion: https://postgr.es/m/458729.1776724816@sss.pgh.pa.us
Backpatch-through: 16, 17, master
The original implementation of remove_rel_from_restrictinfo()
thought it could skate by with removing no-longer-valid relid
bits from only the clause_relids and required_relids fields.
This is quite bogus, although somehow we had not run across a
counterexample before now. At minimum, the left_relids and
right_relids fields need to be fixed because they will be
examined later by clause_sides_match_join(). But it seems
pretty foolish not to fix all the relid fields, so do that.
This needs to be back-patched as far as v16, because the
bug report shows a planner failure that does not occur
before v16. I'm a little nervous about back-patching,
because this could cause unexpected plan changes due to
opening up join possibilities that were rejected before.
But it's hard to argue that this isn't a regression. Also,
the fact that this changes no existing regression test results
suggests that the scope of changes may be fairly narrow.
I'll refrain from back-patching further though, since no
adverse effects have been demonstrated in older branches.
Bug: #19460
Reported-by: François Jehl <francois.jehl@pigment.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/19460-5625143cef66012f@postgresql.org
Backpatch-through: 16
Before each call to the SRF, initialize isnull and isDone, as per the
comments for struct ReturnSetInfo. This fixes a Coverity warning
about rsi.isDone not being initialized. The built-in
{multi,}range_minus_multi functions don't return without setting it,
but a user-supplied function might not be as accommodating.
We also add statistics tracking around the function call, which
will be expected once user-defined withoutPortionProcs functions
are supported, and a cross-check on rsi.returnMode just for
paranoia's sake.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Co-authored-by: Paul A Jungwirth <pj@illuminatedcomputing.com>
Discussion: https://postgr.es/m/4126231.1776622202@sss.pgh.pa.us
Although REPACK (CONCURRENTLY) uses replication slots, there is no
concern that the slot will leak data of other users, because the
MAINTAIN privilege on the table is required anyway; requiring
REPLICATION is user-unfriendly without providing any actual protection.
A related aspect is that the REPLICATION attribute is not needed to
prevent REPACK from stealing slots from logical replication, since
commit e76d8c749c made REPACK use a separate pool of replication
slots.
Similarly, there's no reason to require that the table owner has the
LOGIN privilege. Bypass the default behavior in the background worker
launch sequence.
Because there are now successful concurrent repack runs in the
regression tests, we're forced to run test_plan_advice under
wal_level=replica, so add that. Also, move the cluster.sql test to a
different parallel group in parallel_schedule: apparently the use of the
repack worker causes it to exceed the maximum limit of processes in some
runs (the actual limit reached is the number of XIDs in a snapshot's xip
array).
Author: Antonin Houska <ah@cybertec.at>
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Chao Li <lic@highgo.com>
Discussion: https://postgr.es/m/aeJHPNmL4vVy3oPw@pryzbyj2023
Create the PL/pgSQL function and procedure for the top-level WAIT FOR
checks in a single transaction, then wait once for standby replay before
running both tests. Also revise some surrounding comments.
This avoids an extra 'wait_for_catchup()' on the delayed standby without
changing the test coverage.
Discussion: https://postgr.es/m/CABPTF7WZ1yuYz8V%3Dxsbghg8e7qaAm5MpyNw6BthWcbN7%2BP6biw%40mail.gmail.com
Author: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
The self-join elimination (SJE) commit grafted self-join removal onto
remove_rel_from_query(), which was originally written for left-join
removal only. This resulted in several issues:
- Comments throughout remove_rel_from_query() still assumed only
left-join removal, making the code misleading.
- ChangeVarNodesExtended was called on phv->phexpr with subst=-1
during left-join removal, which is pointless and confusing since any
surviving PHV shouldn't reference the removed rel.
- phinfo->ph_lateral was adjusted for left-join removal, which is
unnecessary since the removed relid cannot appear in ph_lateral for
outer joins.
- The comment about attr_needed reconstruction was in
remove_rel_from_query(), but the actual rebuild is performed by the
callers.
- EquivalenceClass processing in remove_rel_from_query() is redundant
for self-join removal, since the caller (remove_self_join_rel)
already handles ECs via update_eclasses().
- In remove_self_join_rel(), ChangeVarNodesExtended was called on
root->processed_groupClause, which contains SortGroupClause nodes
that have no Var nodes to rewrite. The accompanying comment
incorrectly mentioned "HAVING clause".
This patch fixes all these issues, clarifying the separation between
left-join removal and self-join elimination code paths within
remove_rel_from_query(). The resulting code is also better structured
for adding new types of join removal (such as inner-join removal) in
the future.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Andrei Lepikhov <lepihov@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48JC4OVqE=3gMB6se2WmRNNfMyFyYxm-09vgpm+Vwe8Hg@mail.gmail.com
Parallel apply workers previously failed to report statistics while
waiting for new work in the main loop. This resulted in the stats from the
most recent transaction remaining unbuffered, leading to arbitrary
reporting delays—particularly when streamed transactions were infrequent.
This commit ensures that statistics are explicitly flushed when the worker
is idle, providing timely visibility into accumulated worker activity.
Author: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 16, where it was introduced
Discussion: https://postgr.es/m/TYRPR01MB1419579F217CC4332B615589594202@TYRPR01MB14195.jpnprd01.prod.outlook.com
1. Make it so test_random_operations() can accept a NULL to have the
function select a random seed.
2. Widen the seed parameter of test_random_operations() to bigint.
Without that, it'll be impossible to run the function with a seed
which was selected by GetCurrentTimestamp(), and if a randomly
selected seed ever results in a failure, we'll likely want to run
with the same seed to debug the issue.
3. Report the seed in the error messages in test_random_operations().
If the buildfarm were ever to fail there, we'd certainly want to know
what this was.
4. Add CHECK_FOR_INTERRUPTS() to test_random_operations(). Someone might
run with a large num_ops and they'd have no way to cancel the query.
5. Minor cosmetic fixes; header order and whitespace issue.
To allow #1, the STRICT modifier had to be removed. The additional
prechecks were added as I didn't see how else to handle someone passing
those parameters as NULL.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Greg Burd <greg@burd.me>
Discussion: https://postgr.es/m/CAApHDvrDW9W72vAr7h7XeCu7+Qz-_Vff02Q+RPPuVeM0Qf0MCw@mail.gmail.com
The switch from long to int64 in commit 13b935cd52 was incomplete.
It was shifting the constant 1L, which is not always 64 bit. Fix by
using an explicit int64 constant.
MSVC warning:
../src/backend/utils/hash/dynahash.c(1767): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
Also add the corresponding warning to the standard warning set on
MSVC, to help catch similar issues in the future.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/1142ad86-e475-41b3-aeee-c6ad913064fa%40eisentraut.org
The argument was marked as "const void *X", but that might rightly
give the compiler the idea that *X cannot be modified through the
resulting Datum, and make incorrect optimizations based on that. Some
functions use pointer Datums to pass output arguments, like GIN
support functions. Coverity started to complain after commit
6f5ad00ab7 that there's dead code in ginExtractEntries(), because it
didn't see that it passes PointerGetDatum(&nentries) to a function
that sets it.
This issue goes back to commit c8b2ef05f4 (version 16), which
changed PointerGetDatum() from a macro to a static inline function.
This commit changes it back to a macro, but uses a trick with a dummy
conditional expression to still produce a compiler error if you try to
pass a non-pointer as the argument.
Even though this goes back to v16, I'm only committing this to
'master' for now, to verify that this silences the Coverity warning.
If this works, we might want to introduce separate const and non-const
versions of PointerGetDatum() instead of this, but that's a bigger
patch. It's also not decided yet whether to back-patch this (or some
other fix), given that we haven't yet seen any hard evidence of
compilers actually producing buggy code because of this.
Discussion: https://www.postgresql.org/message-id/342012.1776017102@sss.pgh.pa.us
Previously, tab completion after EXCEPT (...) always suggested FROM SERVER.
This was correct for IMPORT FOREIGN SCHEMA ... EXCEPT (...), but became
incorrect once commit fd366065e0 added CREATE PUBLICATION ... EXCEPT (...).
This commit updates tab completion so FROM SERVER is no longer suggested
after CREATE PUBLICATION ... EXCEPT (...), while preserving the existing
behavior for IMPORT FOREIGN SCHEMA ... EXCEPT (...).
Author: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Shveta Malik <shveta.malik@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CALDaNm1-Fx6Msw6zcRuSjgQdw6asdTyp2DwP-4TCKGYAT+ndsA@mail.gmail.com
An invalid database has datconnlimit set to -2. pg_get_database_ddl()
emits this verbatim as CONNECTION LIMIT = -2, which ALTER DATABASE
rejects. Error out early instead.
Reported-by: Lakshmi N <lakshmin.jhs@gmail.com>
Author: Lakshmi N <lakshmin.jhs@gmail.com>
Reviewed-by: Amit Langote <amitlangote09@gmail.com>
Reviewed-by: Euler Taveira <euler@eulerto.com>
Reviewed-by: Hu Xunqi <huxunqi.08@gmail.com>
Discussion: https://postgr.es/m/CA+3i_M8m1k2gFch+tU0JmAQh9FRV+pFrfTXDrJo+BqmwsTmOhg@mail.gmail.com
Commit 3e2a1496ba made the psql TAP test require the DETAIL line on
platforms with SA_SIGINFO, rather than making it optional. This
unexpectedly blew up on OpenBSD buildfarm members, because OpenBSD does
not set si_pid for SIGTERM signals even though it has SA_SIGINFO
defined.
So revert to the test as it was in commit 55890a9194, where the detail
line being missing never causes an error.
Author: Jakub Wartak <jakub.wartak@enterprisedb.com>
Suggested-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/2007157.1776269052%40sss.pgh.pa.us
The backend running REPACK can check DecodingWorkerShared->initialized
before the worker could have the chance to initialize it, possibly
leading to wrong behavior.
While at it, remove DecodingWorkerShared->worker_dsm_segment, because
that doesn't actually need to be in shared memory; a simple local-memory
global variable is enough.
Oversights in commit 28d534e2ae.
Author: Antonin Houska <ah@cybertec.at>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/18181295-8375-4789-ad32-269d78d6001e@gmail.com
add323da40 started updating the visibility map in the same WAL record
as pruning and freezing. This included updating the freespace map during
replay of a record setting the VM, which we've done since ab7dbd681.
add323da40, however, conditioned doing so on there being > 0 freespace
on the page, which differed from the previous state for records updating
the VM.
The FSM is not WAL-logged and is instead updated heuristically on
standbys. In rare cases, this heuristic could lead to pages with 0
freespace having outdated entries in the FSM. If the standby is later
promoted and vacuum skips these pages because they are marked
all-visible/all-frozen, overly optimistic values would be propagated up
the FSM tree, causing slowness when searching for freespace for new
tuples.
Fix it by always updating the FSM during replay when setting VM bits.
Author: Melanie Plageman <melanieplageman@gmail.com>
Reported-by: Alexey Makhmutov <a.makhmutov@postgrespro.ru>
Discussion: https://postgr.es/m/ead2f110-c736-48f5-99e1-023dc9acbf0b%40postgrespro.ru
Commit a2b02293bc switched various checks to use XLogRecPtrIsValid(),
but later changes reintroduced XLogRecPtrIsInvalid() and direct comparisons
with InvalidXLogRecPtr.
This commit replaces those uses with XLogRecPtrIsValid() for better
readability and consistency.
Author: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Xiaopeng Wang <wxp_728@163.com>
Reviewed-by: Amul Sul <sulamul@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CALDaNm16knMFtcqyAG3XYSkyagmVXfhaR0T=hau8UTAU0+eLQQ@mail.gmail.com
* JOHAB: replace the incorrect "simplified Chinese" description with
a correct one that identifies it as the Korean combining (Johab)
encoding standardized in KS X 1001 annex 3.
* EUC_KR: drop a stray space before the comma in the existing
comment, and note that the encoding covers the KS X 1001
precomposed (Wansung) form.
* UHC: spell out "Unified Hangul Code", clarify that it is
Microsoft Windows CodePage 949, and describe its relationship to
EUC-KR (superset covering all 11,172 precomposed Hangul syllables).
Backpatch-through: 14
Author: Henson Choi <assam258@gmail.com>
Discussion: https://postgr.es/m/CAAAe_zAFz1v-3b7Je4L%2B%3DwZM3UGAczXV47YVZfZi9wbJxspxeA%40mail.gmail.com
The comment on the return-false path when both UNION siblings are
exhausted said "there are more rows," which is the opposite of what
the code does. The code itself is correct, returning false to signal
no more rows, but the misleading comment could tempt a reader into
"fixing" the return value, which would cause UNION plans to loop
indefinitely.
Back-patch to 17, where JSON_TABLE was introduced.
Author: Chuanwen Hu <463945512@qq.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/tencent_4CC6316F02DECA61ACCF22F933FEA5C12806@qq.com
Backpatch-through: 17
Previously, when the walreceiver exited from WalRcvWaitForStartPosition()
at the startup process's request, it called exit(1) directly. This could
skip cleanup performed by the callback functions.
This commit makes the walreceiver to use proc_exit() instead, ensuring
normal cleanup is executed on exit.
Also this commit updates comments describing walreceiver termination.
Apply to master only, as this has not caused practical issues so far.
Author: Chao Li <lic@highgo.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Discussion: https://postgr.es/m/74381238-4E8A-4621-B794-57025DCCE0BA@gmail.com
COPY TO with FORMAT json was including generated columns in the
output, unlike TEXT and CSV formats. Virtual generated columns
appeared as null, and stored ones showed their computed values.
The JSON code path only built a restricted TupleDesc when an explicit
column list was given (attnamelist != NIL), but CopyGetAttnums()
also excludes generated columns from the default list. Fix by
checking whether the attnumlist is shorter than the full TupleDesc
instead.
Bug introduced in 7dadd38cda.
Author: Satya Narlapuram <satya.narlapuram@gmail.com>
Reviewed-by: Jian He <jian.universality@gmail.com>
Discussion: https://postgr.es/m/CAHg+QDcfpGDoPL3fvfjXRtfn=fny6DdJR6BAy6TpS1Xj2EZfXA@mail.gmail.com
Commit 095c9d4cf06 added errdetail() reporting of the PID and UID of
the process that sent a termination signal. However, as noted by
Andres Freund, the implementation had architectural problems:
1. wrapper_handler() in pqsignal.c contained SIGTERM-specific logic
(setting ProcDieSenderPid/Uid), violating its role as a generic
signal dispatch wrapper.
2. Using globals to pass sender info between wrapper_handler and the
real handler is unsafe when signals nest on some platforms.
3. The syncrep.c errdetail used psprintf() to conditionally embed
text via %s, breaking translatability.
Adopt the approach proposed by Andres Freund: introduce a
pg_signal_info struct that is passed as an argument to all signal
handlers via the SIGNAL_ARGS macro. wrapper_handler populates it
from siginfo_t when SA_SIGINFO is available, or with zeros otherwise.
This keeps wrapper_handler fully generic and avoids any globals for
passing signal metadata.
Since pqsigfunc now has a different signature from the system's
signal handler type, SIG_IGN and SIG_DFL can no longer be passed
directly to pqsignal(). Introduce PG_SIG_IGN and PG_SIG_DFL macros
that cast to the new pqsigfunc type, and update all call sites.
The legacy pqsignal() in libpq retains its original signature via
a local typedef.
Only die() reads pg_siginfo today, copying the sender PID/UID into
ProcDieSenderPid/Uid for later use by ProcessInterrupts(). Only the
first SIGTERM's sender info is recorded.
Also fix the syncrep.c translatability issue by using separate ereport
calls with complete, independently translatable errdetail strings.
Also make the psql TAP test require the DETAIL line on platforms with
SA_SIGINFO, rather than making it unconditionally optional.
On Windows, pg_signal_info uses uint32_t for pid and uid fields
since pid_t/uid_t are not available early enough in the include
chain. The Windows signal dispatch in pgwin32_dispatch_queued_signals()
passes a zeroed pg_signal_info to handlers.
Author: Andres Freund <andres@anarazel.de>
Author: Jakub Wartak <jakub.wartak@enterprisedb.com>
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/cwyyryh2veejuxbj5ifzyaejw7jhhqc5mrdeq56xckknsdecn2@6hzfcxde2nm5
Discussion: https://postgr.es/m/jygesyr7mwg7ovdbxpmjvvbi3hccptpkcreqb645h7f56puwbz@hmkkwi3melfe
The NOTNULL_SOURCE_SYSCACHE code path in var_is_nonnullable() used
get_attnotnull() to check pg_attribute.attnotnull, which is true for
both valid and invalid (NOT VALID) NOT NULL constraints. An invalid
constraint does not guarantee the absence of NULLs, so this could lead
to incorrect results. For example, query_outputs_are_not_nullable()
could wrongly conclude that a subquery's output is non-nullable,
causing NOT IN to be incorrectly converted to an anti-join.
Fix by checking the attnullability field in the relation's tuple
descriptor instead, which correctly distinguishes valid from invalid
constraints, consistent with what the NOTNULL_SOURCE_HASHTABLE code
path already does.
While at it, rename NOTNULL_SOURCE_SYSCACHE to NOTNULL_SOURCE_CATALOG
to reflect that this code path no longer uses a syscache lookup, and
remove the now-unused get_attnotnull() function.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48ALW=mR0ydQ62dGS-Q+3D7WdDSh=EWDezcKp19xi=TUA@mail.gmail.com
DatumGetArrayTypeP() can return a pointer into the tuple when the
datum is stored as a short varlena, so pfree() on the result crashes.
Use DatumGetArrayTypePCopy() to always get a palloc'd copy.
Bug introduced in 76e514ebb4 and a4f774cf1c.
Reported-by: Jeff Davis <pgsql@j-davis.com>
Author: Satya Narlapuram <satya.narlapuram@gmail.com>
Discussion: https://postgr.es/m/CAHg+QDdWtv9PKtPZEokwGCNtbv4MVnfYw5wMZrsEj4xizSNe5Q@mail.gmail.com
