3ca43dbbb6 adds regression tests with permission checks. The conflict has
been observed at buildfarm member piculet.
This commit fixes the conflict in the following way.
1. partition_split.sql now uses role names regress_partition_split_alice and
regress_partition_split_bob (it mistakenly used
regress_partition_merge_alice and regress_partition_merge_bob before).
2. Permissions on schemas partitions_merge_schema and partition_split_schema
are granted to corresponding roles. Before, the lack of permissions led to
the creation of objects in the public schema and potential conflict.
Reported-by: Daniel Gustafsson
Discussion: https://postgr.es/m/03A07EF6-98D2-419B-A3AA-A111C64CC207%40yesql.se
There are some problems with the new way to handle these constraints
that were detected at the last minute, and require fixes that appear too
invasive to be doing this late in the cycle. Revert this (again) for
now, we'll try again with these problems fixed.
The following commits are reverted:
b0e96f3119 Catalog not-null constraints
9b581c5341 Disallow changing NO INHERIT status of a not-null constraint
d0ec2ddbe0 Fix not-null constraint test
ac22a9545c Move privilege check to the right place
b0f7dd915b Check stack depth in new recursive functions
3af7217942 Update information_schema definition for not-null constraints
c3709100be Fix propagating attnotnull in multiple inheritance
d9f686a72e Fix restore of not-null constraints with inheritance
d72d32f52d Don't try to assign smart names to constraints
0cd711271d Better handle indirect constraint drops
13daa33fa5 Disallow NO INHERIT not-null constraints on partitioned tables
d45597f72f Disallow direct change of NO INHERIT of not-null constraints
21ac38f498 Fix inconsistencies in error messages
Discussion: https://postgr.es/m/202405110940.joxlqcx4dogd@alvherre.pgsql
This test case intended to fail because setting a column as generated to
the partitioned table while leaving the partition alone is not allowed;
but instead failed because of a discrepancy of not-null constraint. Fix
this by adding the not-null constraint first, then set the column as
generated in a separate ALTER TABLE command, which gets the expected
error. Also, because the next test also wants to set the column as
not-null, add a BEGIN/ROLLBACK block so that the added not-null is
removed.
Oversight in 6995863157.
Currently, we check only owner permission for the parent table before
MERGE/SPLIT partition operations. This leads to a security hole when users
can get access to the data of partitions without permission. This commit
fixes this problem by requiring owner permission on all the partitions
involved.
Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/0520c72e-8d97-245e-53f9-173beca2ab2e%40gmail.com
Author: Dmitry Koval, Alexander Korotkov
On other Windows build farm animals it is already skipped because they
don't use UTF-8 encoding. On "hamerkop", UTF-8 is used, and then the
test fails.
It is not clear to me (a non-Windows person looking only at buildfarm
evidence) whether Windows is less sophisticated than other OSes and
doesn't know how to downcase Turkish İ with the standard Unicode
database, or if it is more sophisticated than other systems and uses
locale-specific behavior like ICU does.
Whichever the reason, the result is the same: we need to skip the test
on Windows, just as we already do for ICU, at least until a
Windows-savvy developer comes up with a better idea. The technique for
detecting the OS is borrowed from collate.windows.win1252.sql.
This was anticipated by commit c2e8bd27, but the problem only surfaced
when Windows build farm animals started using Meson.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CA%2BhUKGJ1LeC3aE2qQYTK95rFVON3ZVoTQpTKJqxkHdtEyawH4A%40mail.gmail.com
This commit fixes a race condition between injection point run and
detach, where a point detached by a backend and concurrently running in
a second backend could cause the second backend to do an incorrect
condition check. This issue happens because the second backend
retrieves the callback information in a first step in the shmem hash
table for injection points, and the condition in a second step within
the callback. If the point is detached between these two steps, the
condition would be removed, causing the point to run while it should
not. Storing the condition in the new private_data area introduced in
33181b48fd ensures that the condition retrieved is consistent with its
callback.
This commit leads to a lot of simplifications in the module
injection_points, as there is no need to handle the runtime conditions
inside it anymore. Runtime conditions have no more a maximum number.
Per discussion with Noah Misch.
Reviewed-by: Noah Misch
Discussion: https://postgr.es/m/20240509031553.47@rfd.leadboat.com
This commit extends the backend-side infrastructure of injection points
so as it becomes possible to register some input data when attaching a
point. This private data can be registered with the function name and
the library name of the callback when attaching a point, then it is
given as input argument to the callback. This gives the possibility for
modules to pass down custom data at runtime when attaching a point
without managing that internally, in a manner consistent with the
callback entry retrieved from the hash shmem table storing the injection
point data.
InjectionPointAttach() gains two arguments, to be able to define the
private data contents and its size.
A follow-up commit will rely on this infrastructure to close a race
condition with the injection point detach in the module
injection_points.
While on it, this changes InjectionPointDetach() to return a boolean,
returning false if a point cannot be detached. This has been mentioned
by Noah as useful when it comes to implement more complex tests with
concurrent point detach, solid with the automatic detach done for local
points in the test module.
Documentation is adjusted in consequence.
Per discussion with Noah Misch.
Reviewed-by: Noah Misch
Discussion: https://postgr.es/m/20240509031553.47@rfd.leadboat.com
A PRIMARY KEY or UNIQUE constraint with WITHOUT OVERLAPS will be a
GiST index, not a B-Tree, but it will still have indisunique set. The
code for ON CONFLICT fails if it sees a non-btree index that has
indisunique. This commit fixes that and adds some tests. But now
that we can't just test indisunique, we also need some extra checks to
prevent DO UPDATE from running against a WITHOUT OVERLAPS constraint
(because the conflict could happen against more than one row, and we'd
only update one).
Author: Paul A. Jungwirth <pj@illuminatedcomputing.com>
Discussion: https://www.postgresql.org/message-id/1426589a-83cb-4a89-bf40-713970c07e63@illuminatedcomputing.com
A lot of the examples in this section are quite ancient and no
longer match the system's current behavior. I ran through them
and updated the sample output to match what I get today.
Notably, it seems that the tenk1 table got a few pages smaller back
around v12, so that a lot of the cost estimates for queries on that
table are a little different now. (This also caused some examples
to surprisingly not match others, depending on when they were made.)
One mergejoin-based example no longer produces the same plan at all,
so I had to work around that, including switching to a different
example of using an enable_xxx setting.
I also changed a couple of examples that were relying on nonexistent
tables or indexes. IMO the premise of this section is that the
examples should be more-or-less reproducible against the regression
database, so that these examples were unhelpful as given. It's not
that hard to find a corresponding example with the tables we do have.
Some of the EXPLAIN ANALYZE examples were out of date in that we show
more numbers than we did at the time.
Possibly chapter 68 could use similar review, but I'm out of
energy for today.
If we recursed to a new call of the same function, with a different
coldeflist (AS clause), it would fail because the inner call would
overwrite the outer call's idea of what to return. This is vaguely
like 1d2fe56e4 and c5bec5426, but it's not due to any API decisions:
it's just that we computed the actual output rowtype at the start of
the call, and saved it in the per-procedure data structure. We can
fix it at basically zero cost by doing the computation at the end
of each call instead of the start.
It's not clear that there's any real-world use-case for such a
function, but given that it doesn't cost anything to fix,
it'd be silly not to.
Per report from Andreas Karlsson. Back-patch to all supported
branches.
Discussion: https://postgr.es/m/1651a46d-3c15-4028-a8c1-d74937b54e19@proxel.se
It turns out that we broke this in commit e5bc9454e, because
the code was assuming that no dependent types would appear
among the extension's direct dependencies, and now they do.
This isn't terribly hard to fix: just skip dependent types,
expecting that we will recurse to them when we process the parent
object (which should also be among the direct dependencies).
But a little bit of refactoring is needed so that we can avoid
duplicating logic about what is a dependent type.
Although there is some testing of ALTER EXTENSION SET SCHEMA,
it failed to cover interesting cases, so add more tests.
Discussion: https://postgr.es/m/930191.1715205151@sss.pgh.pa.us
The initial building of RestrictInfos and SpecialJoinInfos tends to
create structures that share relid sets (such as syn_lefthand and
outer_relids). There's nothing wrong with that in itself, but when
we modify those relid sets during join removal, we have to be sure
not to corrupt the values that other structures are pointing at.
remove_rel_from_query() was careless about this. It accidentally
worked anyway because (1) we'd never be reducing the sets to empty,
so they wouldn't get pfree'd; and (2) the in-place modification is the
same one that we did or will apply to the other struct's relid set,
so that there wasn't visible corruption at the end of the process.
While there's no live bug in a standard build, of course this is way
too fragile to accept going forward. (Maybe we should back-patch
this change too for safety, but I've refrained for now.)
This bug was exposed by the recent invention of REALLOCATE_BITMAPSETS.
Commit e0477837c had installed a fix, but that went away with the
reversion of SJE, so now we need to fix it again.
David Rowley and Tom Lane
Discussion: https://postgr.es/m/CACJufxFVQmr4=JWHAOSLuKA5Zy9H26nY6tVrRFBOekHoALyCkQ@mail.gmail.com
json_lex_string() relies on pg_encoding_mblen_bounded() to point to the
end of a JSON string when generating an error message, and the input it
uses is not guaranteed to be null-terminated.
It was possible to walk off the end of the input buffer by a few bytes
when the last bytes consist of an incomplete multi-byte sequence, as
token_terminator would point to a location defined by
pg_encoding_mblen_bounded() rather than the end of the input. This
commit switches token_terminator so as the error uses data up to the
end of the JSON input.
More work should be done so as this code could rely on an equivalent of
report_invalid_encoding() so as incorrect byte sequences can show in
error messages in a readable form. This requires work for at least two
cases in the JSON parsing API: an incomplete token and an invalid escape
sequence. A more complete solution may be too invasive for a backpatch,
so this is left as a future improvement, taking care of the overread
first.
A test is added on HEAD as test_json_parser makes this issue
straight-forward to check.
Note that pg_encoding_mblen_bounded() no longer has any callers. This
will be removed on HEAD with a separate commit, as this is proving to
encourage unsafe coding.
Author: Jacob Champion
Discussion: https://postgr.es/m/CAOYmi+ncM7pwLS3AnKCSmoqqtpjvA8wmCdoBtKA3ZrB2hZG6zA@mail.gmail.com
Backpatch-through: 13
User-written triggers can modify or block the effects of SQL update
and delete operations. That includes operations that are executed
to implement foreign keys' referential integrity actions (such as
ON UPDATE SET NULL or ON DELETE CASCADE). Therefore it's possible
for a misdesigned trigger to result in a database state that violates
the foreign key constraint.
While this isn't great, the alternatives seem worse: in particular,
refusing to fire triggers for such updates would break many valuable
use-cases. We could also try to recheck the constraint after the
action, but that'd roughly double the already-high cost of FK
constraint enforcement, for no benefit in normal cases. So we've
always considered that it's on the trigger programmer's head to
avoid breaking RI actions. This was never documented anywhere,
though. Add a para to the Triggers chapter to explain it.
Laurenz Albe, David Johnston, Tom Lane
Discussion: https://postgr.es/m/b81fe38fcc25a81be6e2e5b3fc1ff624130762fa.camel@cybertec.at
If -l was specified together with selective-restore options such as -n
or -N, dependent TOC entries such as comments would be omitted from
the listing, even when an actual restore would have selected them.
This happened because PrintTOCSummary neglected to update the te->reqs
marking of the entry they depended on.
Per report from Justin Pryzby. This has been wrong since 0d4e6ed30
taught _tocEntryRequired to sometimes look at the "reqs" marking of
other TOC entries, so back-patch to all supported branches.
Discussion: https://postgr.es/m/ZjoeirG7yxODdC4P@pryzbyj2023
If a plpython-language trigger caused another one to be invoked,
the "TD" dictionary created for the inner one would overwrite the
outer one's "TD" dictionary. This is more or less the same problem
that 1d2fe56e4 fixed for ordinary functions in plpython, so fix it
the same way, by saving and restoring "TD" during a recursive
invocation.
This fix makes an ABI-incompatible change in struct PLySavedArgs.
I'm not too worried about that because it seems highly unlikely that
any extension is messing with those structs. We could imagine doing
something weird to preserve nominal ABI compatibility in the back
branches, like keeping the saved TD object in an extra element of
namedargs[]. However, that would only be very nominal compatibility:
if anything *is* touching PLySavedArgs, it would likely do the wrong
thing due to not knowing about the additional value. So I judge it
not worth the ugliness to do something different there.
(I also changed struct PLyProcedure, but its added field fits
into formerly-padding space, so that should be safe.)
Per bug #18456 from Jacques Combrink. This bug is very ancient,
so back-patch to all supported branches.
Discussion: https://postgr.es/m/3008982.1714853799@sss.pgh.pa.us
When changing the data type of a column of a partitioned table, craft
the ALTER SEQUENCE command only once. Partitions do not have identity
sequences of their own and thus do not need a ALTER SEQUENCE command
for each partition.
Fix getIdentitySequence() to fetch the identity sequence associated
with the top-level partitioned table when a Relation of a partition is
passed to it. While doing so, translate the attribute number of the
partition into the attribute number of the partitioned table.
Author: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Dmitry Dolgov <9erthalion6@gmail.com>
Discussion: https://www.postgresql.org/message-id/3b8a9dc1-bbc7-0ef5-6863-c432afac7d59@gmail.com
The executor only supports CurrentOfExpr as the sole tidqual of a
TidScan plan node. tidpath.c failed to take any particular care about
that, but would just take the first ctid equality qual it could find
in the target relation's baserestrictinfo list. Originally that was
fine because the grammar prevents any other WHERE conditions from
being combined with CURRENT OF <cursor>. However, if the relation has
RLS visibility policies then those would get included in the list.
Should such a policy include a condition on ctid, we'd typically grab
the wrong qual and produce a malfunctioning plan.
To fix, introduce a simplistic priority ordering scheme for which ctid
equality qual to prefer. Real-world cases involving more than one
such qual are so rare that it doesn't seem worth going to any great
trouble to choose one over another, so I didn't work very hard; but
this code could be extended in future if someone thinks differently.
It's extremely difficult to think of a reasonable use-case for an RLS
restriction involving ctid, and certainly we've heard no field reports
of this failure. So this doesn't seem worthy of back-patching, but
in the name of cleanliness let's fix it going forward.
Patch by me, per report from Robert Haas.
Discussion: https://postgr.es/m/3914881.1715038270@sss.pgh.pa.us
Both the pgbench --help output and the reference page have sections
for initialization options, benchmarking options, and common options.
But the --debug option ended up in the wrong place on the reference
page. Fix that by making the documentation match the --help output.
The code change this made might well be fine to keep, but the
comment justifying it by reference to self-join removal isn't.
Let's just go back to the status quo ante, pending a more thorough
review/redesign of SJE.
(I found this by grepping to see if any references to self-join
removal remained in the tree.)
The catalog view pg_stats_ext fails to consider privileges for
expression statistics. The catalog view pg_stats_ext_exprs fails
to consider privileges and row-level security policies. To fix,
restrict the data in these views to table owners or roles that
inherit privileges of the table owner. It may be possible to apply
less restrictive privilege checks in some cases, but that is left
as a future exercise. Furthermore, for pg_stats_ext_exprs, do not
return data for tables with row-level security enabled, as is
already done for pg_stats_ext.
On the back-branches, a fix-CVE-2024-4317.sql script is provided
that will install into the "share" directory. This file can be
used to apply the fix to existing clusters.
Bumps catversion on 'master' branch only.
Reported-by: Lukas Fittl
Reviewed-by: Noah Misch, Tomas Vondra, Tom Lane
Security: CVE-2024-4317
Backpatch-through: 14
Both the initdb --help output and the reference page have a section
for options and a section for less commonly used options. But some
recently added options were sprinkled around inconsistently. Fix that
by making the documentation match the --help output.
Injection points created under injection_points_set_local() are cleaned
up by a shmem_exit() callback. The spinlock used by the module would
be hold while calling InjectionPointDetach(), which is incorrect as
spinlocks should avoid external calls while hold.
This commit changes the shmem_exit() callback to detach the points in
three steps with the spinlock acquired twice, knowing that the
injection points should be around with the conditions related to them:
- Scans for the points to detach in a first loop, while holding the
spinlock.
- Detach them.
- Remove the registered conditions.
It is still possible for other processes to detach local points
concurrently of the callback. I have wanted to restrict the detach, but
Noah has mentioned that he has in mind some cases that may require this
capability. No tests in the tree based on injection points need that
currently.
Thinko in f587338dec.
Reported-by: Noah Misch
Reviewed-by: Noah Misch
Discussion: https://postgr.es/m/20240501231214.40@rfd.leadboat.com
If pg_init_privs were to contain a NULL ACL field, this code would
pass old_acl == NULL to merge_acl_with_grant, which would crash.
The case shouldn't happen, but it just takes a couple more lines
of code to guard against it, so do so.
Oversight in 534287403; no back-patch needed.
94985c210 added code to detect when WindowFuncs were monotonic and
allowed additional quals to be "pushed down" into the subquery to be
used as WindowClause runConditions in order to short-circuit execution
in nodeWindowAgg.c.
The Node representation of runConditions wasn't well selected and
because we do qual pushdown before planning the subquery, the planning
of the subquery could perform subquery pull-up of nested subqueries.
For WindowFuncs with args, the arguments could be changed after pushing
the qual down to the subquery.
This was made more difficult by the fact that the code duplicated the
WindowFunc inside an OpExpr to include in the WindowClauses runCondition
field. This could result in duplication of subqueries and a pull-up of
such a subquery could result in another initplan parameter being issued
for the 2nd version of the subplan. This could result in errors such as:
ERROR: WindowFunc not found in subplan target lists
To fix this, we change the node representation of these run conditions
and instead of storing an OpExpr containing the WindowFunc in a list
inside WindowClause, we now store a new node type named
WindowFuncRunCondition within a new field in the WindowFunc. These get
transformed into OpExprs later in planning once subquery pull-up has been
performed.
This problem did exist in v15 and v16, but that was fixed by 9d36b883b
and e5d20bbd.
Cat version bump due to new node type and modifying WindowFunc struct.
Bug: #18305
Reported-by: Zuming Jiang
Discussion: https://postgr.es/m/18305-33c49b4c830b37b3%40postgresql.org
Commit 619bc23a1 changed "make dist" to invoke "git archive",
but hard-wired the call to specify that the HEAD revision should
be packaged. Our tarball building process needs to be able to
specify which git commit to package (notably, for packaging
back branches). While we could make that work with some hackery
to operate in detached-HEAD state, it's a lot nicer just to expose
git archive's ability to specify what to package. Hence, invent
a new make variable PG_GIT_REVISION. This is undocumented, but
so is "make dist".
Also make corresponding changes in the meson scripts. We have no
near-term intention of using that for package building, but it
will likely happen eventually, so stay prepared.
Discussion: https://postgr.es/m/3552543.1713909947@sss.pgh.pa.us
Alexander Korotkov