Re: MERGE issues around inheritance
On Mon, 26 May 2025 at 12:50, Tender Wang wrote: > >> > "it is possible for the parent to be excluded from the >> > plan and so all of the entries in the resultRelInfo array may be for >> > different relations than rootResultRelInfo." >> > >> > I didn't fully understand the above sentence. Can you give me more >> > information or an example? >> > If the parent is excluded from the plan, the first entry in the >> > resultRelInfo array will not be the parent but some surviving child. >> >> There's an example in the updated regression tests. A non-inherited >> CHECK constraint on the parent causes the planner to exclude the >> parent from the relations being scanned and from the resultRelInfo >> array, so the first resultRelInfo entry is for a child relation. > > Yes, it is. I debugged the updated regression tests. It would crash if > resultRelInfo were used instead of rootResultInfo. > Is that the reason that we must use rootResultInfo? > Yes. To work correctly for an inherited table, ExecInsert() must be passed a pointer to a ResultRelInfo structure that points to the parent table. As I mentioned before, this goes back to commit 387f9ed0a08, so it's worth reading that in more detail. Prior to commit 387f9ed0a08, rootResultInfo was equal to resultRelInfo for an inherited table, which was wrong because that could point to a child table, if the parent table was excluded from the plan. Commit 387f9ed0a08 fixed that by changing the planner so that it set ModifyTable.rootRelation to the index of the parent table, if it was an inherited table. As a result, starting from that commit, for an inherited table, rootResultInfo is a different ResultRelInfo structure which always points to the parent table, making it the correct thing to pass to ExecInsert() under all circumstances. The thing that was overlooked was that the separate ResultRelInfo structure in rootResultInfo, and the insert projection, weren't being correctly initialised for MERGE, which is what this patch aims to fix. Unless there are any further comments, I plan to commit it in a day or so. Regards, Dean
Re: MERGE issues around inheritance
Dean Rasheed 于2025年5月28日周三 18:26写道： > On Mon, 26 May 2025 at 12:50, Tender Wang wrote: > > > >> > "it is possible for the parent to be excluded from the > >> > plan and so all of the entries in the resultRelInfo array may be for > >> > different relations than rootResultRelInfo." > >> > > >> > I didn't fully understand the above sentence. Can you give me more > information or an example? > >> > If the parent is excluded from the plan, the first entry in the > resultRelInfo array will not be the parent but some surviving child. > >> > >> There's an example in the updated regression tests. A non-inherited > >> CHECK constraint on the parent causes the planner to exclude the > >> parent from the relations being scanned and from the resultRelInfo > >> array, so the first resultRelInfo entry is for a child relation. > > > > Yes, it is. I debugged the updated regression tests. It would crash if > resultRelInfo were used instead of rootResultInfo. > > Is that the reason that we must use rootResultInfo? > > > > Yes. To work correctly for an inherited table, ExecInsert() must be > passed a pointer to a ResultRelInfo structure that points to the > parent table. > > As I mentioned before, this goes back to commit 387f9ed0a08, so it's > worth reading that in more detail. > > Prior to commit 387f9ed0a08, rootResultInfo was equal to resultRelInfo > for an inherited table, which was wrong because that could point to a > child table, if the parent table was excluded from the plan. > > Commit 387f9ed0a08 fixed that by changing the planner so that it set > ModifyTable.rootRelation to the index of the parent table, if it was > an inherited table. As a result, starting from that commit, for an > inherited table, rootResultInfo is a different ResultRelInfo structure > which always points to the parent table, making it the correct thing > to pass to ExecInsert() under all circumstances. > Yeah. Your explanation above made me understand completely. Thanks for your explanation. > > The thing that was overlooked was that the separate ResultRelInfo > structure in rootResultInfo, and the insert projection, weren't being > correctly initialised for MERGE, which is what this patch aims to fix. > Yes, it is. > Unless there are any further comments, I plan to commit it in a day or > so. > I don't have any other comments. It looks good for me. -- Thanks, Tender Wang
Re: Foreign key validation failure in 18beta1
Antonin Houska 于2025年5月28日周三 15:51写道： > I've come across an unexpected ERROR during validation of FK constraint in > PG > 18beta1. The same works in PG 17: > > drop table if exists fk; > drop table if exists pk; > create table pk(i int primary key) partition by range (i); > create table pk_1 partition of pk for values from (0) to (1); > create table pk_2 partition of pk for values from (1) to (2); > insert into pk values (0), (1); > create table fk(i int); > insert into fk values (1); > > -- Works if the FK constraint is created as valid. > --alter table fk add foreign key(i) references pk; > > -- Fails if the FK constraint is created as NOT VALID and validated > -- afterwards. > alter table fk add foreign key(i) references pk not valid; > alter table fk validate constraint fk_i_fkey; > git bisect shows since below commit, the failure started. commit b663b9436e7509b5e73c8c372539f067cd6e66c1 Author: Álvaro Herrera Date: Thu Jan 23 15:54:38 2025 +0100 Allow NOT VALID foreign key constraints on partitioned tables -- Thanks, Tender Wang
Re: Standardize the definition of the subtype field of AlterDomainStmt
On 27.05.25 05:06, Quan Zongliang wrote:
I noticed that the subtype of AlterDomainStmt is directly using
constants in the code. It is not conducive to the maintenance and
reading of the code. Based on the definition of AlterTableType, use
"AD_" as the prefix. Define several macros to replace the original
characters.
The subtype of AlterTableCmd is defined using an enumeration. The
subtypes of AlterDomainStmt are relatively few in number, and the
original definition uses characters. These definitions still use
characters and maintain the values unchanged. If some plugins or tools
are also processing AlterDomainStmt, there will be no errors.
You can still make it an enum and assign the currently in use values to
the new symbols, like
enum AlterDomainType
{
AD_AlterDefault = 'T',
AD_DropNotNull = 'N',
...
I would prefer that.
ALTER TABLE ALTER CONSTRAINT misleading error message
hi.
create table t(a int, constraint cc check(a = 1));
ALTER TABLE t ALTER CONSTRAINT cc not valid;
ERROR: FOREIGN KEY constraints cannot be marked NOT VALID
LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid;
^
the error message seems misleading,
should we consider it as a bug for pg18?
the entry point is in gram.y, following part:
| ALTER CONSTRAINT name ConstraintAttributeSpec
{
AlterTableCmd *n = makeNode(AlterTableCmd);
ATAlterConstraint *c = makeNode(ATAlterConstraint);
n->subtype = AT_AlterConstraint;
n->def = (Node *) c;
c->conname = $3;
if ($4 & (CAS_NOT_ENFORCED | CAS_ENFORCED))
c->alterEnforceability = true;
if ($4 & (CAS_DEFERRABLE | CAS_NOT_DEFERRABLE |
CAS_INITIALLY_DEFERRED | CAS_INITIALLY_IMMEDIATE))
c->alterDeferrability = true;
if ($4 & CAS_NO_INHERIT)
c->alterInheritability = true;
processCASbits($4, @4, "FOREIGN KEY",
&c->deferrable,
&c->initdeferred,
&c->is_enforced,
NULL,
&c->noinherit,
yyscanner);
$$ = (Node *) n;
}
Re: Reduce "Var IS [NOT] NULL" quals during constant folding
On Thu, May 22, 2025 at 11:51 PM Tom Lane wrote: > BTW, in my mind the current thread is certainly v19 material, > so I have not looked at Richard's patch yet. Yeah, this patchset is targeted for v19. Maybe we could be more aggressive and have 0001 and 0002 in v18? (no chance for 0003 though) This patchset does not apply anymore due to 2c0ed86d3. Here is a new rebase. Thanks Richard v5-0001-Expand-virtual-generated-columns-before-sublink-p.patch Description: Binary data v5-0002-Centralize-collection-of-catalog-info-needed-earl.patch Description: Binary data v5-0003-Reduce-Var-IS-NOT-NULL-quals-during-constant-fold.patch Description: Binary data
Re: ALTER TABLE ALTER CONSTRAINT misleading error message
jian he 于2025年5月28日周三 17:10写道： > hi. > > create table t(a int, constraint cc check(a = 1)); > ALTER TABLE t ALTER CONSTRAINT cc not valid; > ERROR: FOREIGN KEY constraints cannot be marked NOT VALID > LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid; > ^ > > the error message seems misleading, > should we consider it as a bug for pg18? > the entry point is in gram.y, following part: > It looks like a bug, because constraint cc is not a FOREIGN KEY constraint. -- Thanks, Tender Wang
Re: PG 18 release notes draft committed
For "Improve the speed of multiplication", I think it should say "numeric multiplication" rather than simply "multiplication", and I think it's worth also linking to commits ca481d3c9ab and c4e44224cf6 which were part of the same work. I think it's also worth mentioning 9428c001f67, which sped up numeric division. That can be included in the same item, as in the attached patch, unless you think it's worth listing it as a separate item. Regards, Dean diff --git a/doc/src/sgml/release-18.sgml b/doc/src/sgml/release-18.sgml new file mode 100644 index 619592b..536d7c0 --- a/doc/src/sgml/release-18.sgml +++ b/doc/src/sgml/release-18.sgml @@ -2905,13 +2905,22 @@ Add ARM Neon and SVE CPU intrinsics for -Improve the speed of multiplication (Joel Jacobson, Dean Rasheed) +Improve the speed of numeric multiplication and division (Joel Jacobson, Dean Rasheed) +§ +§ § +§
Re: Suggestion : support for environment variable in initdb to set the superuser password
On 27.05.25 11:43, Reda Agaoua wrote: I do believe it can be useful in a variety of settings, but I'm not sure whether this is secure. Specifically, the documentation advises against using PGPASSWORD for connecting to postgres : "Use of this environment variable is not recommended for security reasons, as some operating systems allow non-root users to see process environment variables via ps; instead consider using a password file (see Section 32.16)." (32.15. Environment Variables) In my opinion, the context for using PGPASSWORD (i.e. connecting to an instance) is very different from that of initdb, where the password is only used once during cluster initialization. So I think the security concerns from section 32.16 may not necessarily apply here. Well, insecure is insecure. "Insecure, but it's ok because it's not used very often" is not a valid excuse.
Re: Standardize the definition of the subtype field of AlterDomainStmt
Peter Eisentraut 于2025年5月28日周三 19:23写道：
> On 27.05.25 05:06, Quan Zongliang wrote:
> > I noticed that the subtype of AlterDomainStmt is directly using
> > constants in the code. It is not conducive to the maintenance and
> > reading of the code. Based on the definition of AlterTableType, use
> > "AD_" as the prefix. Define several macros to replace the original
> > characters.
> > The subtype of AlterTableCmd is defined using an enumeration. The
> > subtypes of AlterDomainStmt are relatively few in number, and the
> > original definition uses characters. These definitions still use
> > characters and maintain the values unchanged. If some plugins or tools
> > are also processing AlterDomainStmt, there will be no errors.
>
> You can still make it an enum and assign the currently in use values to
> the new symbols, like
>
> enum AlterDomainType
> {
> AD_AlterDefault = 'T',
> AD_DropNotNull = 'N',
> ...
>
> I would prefer that.
+1
--
Thanks,
Tender Wang
Re: Add CHECK_FOR_INTERRUPTS in polling loop code path in XactLockTableWait
On 2025/05/27 4:43, Kevin K Biju wrote: Hi Fujii, Thanks for the review. So unless there are any objections, I'm planning to commit the patch with the following commit message. --- Make XactLockTableWait() and ConditionalXactLockTableWait() interruptable more. Previously, XactLockTableWait() and ConditionalXactLockTableWait() could enter a non-interruptible loop when they successfully acquired a lock on a transaction but the transaction still appeared to be running. Since this loop continued until the transaction completed, it could result in long, uninterruptible waits. Although this scenario is generally unlikely since a transaction lock is basically acquired only when the transaction is not running, it can occur in a hot standby. In such cases, the transaction may still appear active due to the KnownAssignedXids list, even while no lock on the transaction exists. For example, this situation can happen when creating a logical replication slot on a standby. The cause of the non-interruptible loop was the absence of CHECK_FOR_INTERRUPTS() within it. This commit adds CHECK_FOR_INTERRUPTS() to the loop in both functions, ensuring they can be interrupted safely. Back-patch to all supported branches. Author: Kevin K Biju Reviewed-by: Fujii Masao Discussion: https://postgr.es/m/CAM45KeELdjhS-rGuvN=zlj_asvzacucz9lzwvzh7bgcd12d...@mail.gmail.com Backpatch-through: 13 --- > Just an idea: how about calling pgstat_report_wait_start() and _end() around pg_usleep(), similar to what WaitExceedsMaxStandbyDelay() does? I was thinking about this as well, but the question is what do we report the wait event here as? The one that makes sense to me is PG_WAIT_LOCK, but that wouldn't align with what pg_locks would display since there is no actual lock grabbed on the standby. I couldn't find an existing wait event that fits this case, so how about adding a new one, like IPC/XactDone, to indicate "Waiting for the transaction to commit or abort"? > Also, would waiting only 1ms per loop cycle be too aggressive? I agree that it's aggressive for this case. But XactLockTableWait/ConditionalXactLockTableWait seem to be used in other places including in heapam so I'm hesitant on changing this behaviour for all of them. I agree that we need more time to consider whether this change is safe. Regards, -- Fujii Masao NTT DATA Japan Corporation
Re: finish TODOs in to_json_is_immutable, to_jsonb_is_immutable also add tests on it
On Mon, May 19, 2025 at 9:09 AM jian he wrote:
>
> hi.
>
> somehow, I accidentally saw the TODOs (commits [3]) on jsonb.c and json.c
> for functions: to_json_is_immutable and to_jsonb_is_immutable.
> The attached patch is to finalize these TODOs.
>
> per coverage [1], [2], there was zero coverage for these two functions.
> so I also added extensive tests on it.
>
I didn't include "utils/rel.h", so v1-0001 won't compile.
The tests were overly verbose,
so I removed some unnecessary ones to simplify them.
From 1c1a162ee9294475ad8beb3afd732fc6ae073b6e Mon Sep 17 00:00:00 2001
From: jian he
Date: Wed, 28 May 2025 14:50:23 +0800
Subject: [PATCH v2 1/1] enhance json_array, json_object expression is
immutable or not
this will make to_json_is_immutable, to_jsonb_is_immutable recurse to
composite data type or array type elements. also add extensive regress tests
for it.
discussion: https://postgr.es/m/CACJufxFz%3DOsXQdsMJ-cqoqspD9aJrwntsQP-U2A-UaV_M%2B-S9g%40mail.gmail.com
---
src/backend/optimizer/util/clauses.c | 10 ++-
src/backend/utils/adt/json.c | 68 ++
src/backend/utils/adt/jsonb.c | 69 +++
src/include/utils/json.h | 2 +-
src/include/utils/jsonb.h | 2 +-
src/test/regress/expected/sqljson.out | 99 +++
src/test/regress/sql/sqljson.sql | 62 +
7 files changed, 281 insertions(+), 31 deletions(-)
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 26a3e050086..f69c68ee15c 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -406,10 +406,14 @@ contain_mutable_functions_walker(Node *node, void *context)
foreach(lc, ctor->args)
{
Oid typid = exprType(lfirst(lc));
+ bool contain_mutable = false;
- if (is_jsonb ?
-!to_jsonb_is_immutable(typid) :
-!to_json_is_immutable(typid))
+ if (is_jsonb)
+to_jsonb_is_immutable(typid, &contain_mutable);
+ else
+to_json_is_immutable(typid, &contain_mutable);
+
+ if(contain_mutable)
return true;
}
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index 51452755f58..36b6920e850 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -13,6 +13,7 @@
*/
#include "postgres.h"
+#include "access/relation.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
#include "common/hashfn.h"
@@ -28,6 +29,7 @@
#include "utils/json.h"
#include "utils/jsonfuncs.h"
#include "utils/lsyscache.h"
+#include "utils/rel.h"
#include "utils/typcache.h"
@@ -692,15 +694,56 @@ row_to_json_pretty(PG_FUNCTION_ARGS)
/*
* Is the given type immutable when coming out of a JSON context?
*
- * At present, datetimes are all considered mutable, because they
- * depend on timezone. XXX we should also drill down into objects
- * and arrays, but do not.
+ * At present, datetimes are all considered mutable, because they depend on
+ * timezone.
*/
-bool
-to_json_is_immutable(Oid typoid)
+void
+to_json_is_immutable(Oid typoid, bool *contain_mutable)
{
+ char att_typtype = get_typtype(typoid);
JsonTypeCategory tcategory;
Oid outfuncoid;
+ Oid att_typelem;
+
+ /* since this function recurses, it could be driven to stack overflow */
+ check_stack_depth();
+
+ Assert(contain_mutable != NULL);
+
+ if (att_typtype == TYPTYPE_DOMAIN)
+ to_json_is_immutable(getBaseType(typoid), contain_mutable);
+ else if (att_typtype == TYPTYPE_COMPOSITE)
+ {
+ /*
+ * For a composite type, recurse into its attributes.
+ */
+ Relation relation;
+ TupleDesc tupdesc;
+ int i;
+
+ relation = relation_open(get_typ_typrelid(typoid), AccessShareLock);
+
+ tupdesc = RelationGetDescr(relation);
+
+ for (i = 0; i < tupdesc->natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
+
+ if (attr->attisdropped)
+continue;
+ to_json_is_immutable(attr->atttypid, contain_mutable);
+ }
+ relation_close(relation, AccessShareLock);
+ }
+ else if (att_typtype == TYPTYPE_RANGE)
+ {
+ to_json_is_immutable(get_range_subtype(typoid), contain_mutable);
+ }
+ else if (OidIsValid((att_typelem = get_element_type(typoid
+ {
+ /* recurse into array element type */
+ to_json_is_immutable(att_typelem, contain_mutable);
+ }
json_categorize_type(typoid, false, &tcategory, &outfuncoid);
@@ -710,26 +753,25 @@ to_json_is_immutable(Oid typoid)
case JSONTYPE_JSON:
case JSONTYPE_JSONB:
case JSONTYPE_NULL:
- return true;
+ break;
case JSONTYPE_DATE:
case JSONTYPE_TIMESTAMP:
case JSONTYPE_TIMESTAMPTZ:
- return false;
+ *contain_mutable = true;
+ break;
case JSONTYPE_ARRAY:
- return false; /* TODO recurse into elements */
-
case JSONTYPE_COMPOSITE:
- return false; /* TODO recurse into fields */
+ break;
case JSONTYPE_NUMERIC:
case JSONTYPE_CAST:
case JSONTYPE_OTHER:
- return func_volatile(outfuncoid) == PROVOLATILE_IMMUTABLE;
+ if
Foreign key validation failure in 18beta1
I've come across an unexpected ERROR during validation of FK constraint in PG 18beta1. The same works in PG 17: drop table if exists fk; drop table if exists pk; create table pk(i int primary key) partition by range (i); create table pk_1 partition of pk for values from (0) to (1); create table pk_2 partition of pk for values from (1) to (2); insert into pk values (0), (1); create table fk(i int); insert into fk values (1); -- Works if the FK constraint is created as valid. --alter table fk add foreign key(i) references pk; -- Fails if the FK constraint is created as NOT VALID and validated -- afterwards. alter table fk add foreign key(i) references pk not valid; alter table fk validate constraint fk_i_fkey; -- Antonin Houska Web: https://www.cybertec-postgresql.com
Re: ALTER DOMAIN ADD NOT NULL NOT VALID
On Thu, May 22, 2025 at 10:08 PM Quan Zongliang wrote: > > It makes sense to support the "NOT NULL NOT VALID" option. > > The two if statements in the AlterDomainNotNull() should be adjusted. > > if (typTup->typnotnull == notNull && !notNull) > ==> > if (!notNull && !typTup->typnotnull) > > > if (typTup->typnotnull == notNull && notNull) > ==> > if (notNull && typTup->typnotnull) > yech, you are right. There is one failed test [0] because of query result order, so I add "ORDER BY conname COLLATE "C";" to stabilize tests. [0] https://api.cirrus-ci.com/v1/artifact/task/6676676240736256/testrun/build/testrun/pg_upgrade/002_pg_upgrade/data/regression.diffs From a840278f579b57cab4c13b91abfcc4f44a6d8a83 Mon Sep 17 00:00:00 2001 From: jian he Date: Wed, 28 May 2025 15:32:03 +0800 Subject: [PATCH v2 1/1] ALTER DOMAIN ADD NOT NULL NOT VALID we have NOT NULL NO VALID for table constraints, we can make domain have NOT VALID not-null constraint too. ALTER DOMAIN SET NOT NULL works similarly to its ALTER TABLE counterpart. It validates an existing invalid NOT NULL constraint and updates pg_constraint.convalidated to true. ALTER DOMAIN ADD NOT NULL will add a new, valid not-null constraint. If the domain already has an NOT VALID not-null constraint, the command will result in an error. If domain already have VALID not-null, add a NOT VALID is no-op. \dD changes: for domain's not null not valid constraint: now column "Nullable" will display as "not null not valid". domain valid not-null constraint works as before. discussed: https://postgr.es/m/CACJufxGcABLgmH951SJkkihK+FW8KR3=odbhxevcf9atqbu...@mail.gmail.com --- doc/src/sgml/catalogs.sgml | 1 + doc/src/sgml/ref/alter_domain.sgml | 1 - src/backend/catalog/pg_constraint.c | 10 +-- src/backend/commands/typecmds.c | 116 +-- src/backend/parser/gram.y| 8 +- src/bin/pg_dump/pg_dump.c| 27 +-- src/bin/pg_dump/t/002_pg_dump.pl | 16 src/bin/psql/describe.c | 4 +- src/test/regress/expected/domain.out | 41 ++ src/test/regress/sql/domain.sql | 22 + 10 files changed, 219 insertions(+), 27 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index cbd4e40a320..fd1b4b0a563 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -9515,6 +9515,7 @@ SCRAM-SHA-256$:&l typnotnull represents a not-null constraint on a type. Used for domains only. + Note: the not-null constraint on domain may not validated. diff --git a/doc/src/sgml/ref/alter_domain.sgml b/doc/src/sgml/ref/alter_domain.sgml index 7485517..4807116eb05 100644 --- a/doc/src/sgml/ref/alter_domain.sgml +++ b/doc/src/sgml/ref/alter_domain.sgml @@ -92,7 +92,6 @@ ALTER DOMAIN name valid using ALTER DOMAIN ... VALIDATE CONSTRAINT. Newly inserted or updated rows are always checked against all constraints, even those marked NOT VALID. - NOT VALID is only accepted for CHECK constraints. diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 2d5ac1ea813..27d2fee52d3 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -651,8 +651,8 @@ findNotNullConstraint(Oid relid, const char *colname) } /* - * Find and return the pg_constraint tuple that implements a validated - * not-null constraint for the given domain. + * Find and return the pg_constraint tuple that implements not-null constraint + * for the given domain. it may be NOT VALID. */ HeapTuple findDomainNotNullConstraint(Oid typid) @@ -675,13 +675,9 @@ findDomainNotNullConstraint(Oid typid) { Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(conTup); - /* - * We're looking for a NOTNULL constraint that's marked validated. - */ + /* We're looking for a NOTNULL constraint */ if (con->contype != CONSTRAINT_NOTNULL) continue; - if (!con->convalidated) - continue; /* Found it */ retval = heap_copytuple(conTup); diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index 45ae7472ab5..12744e906ec 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -2757,12 +2757,70 @@ AlterDomainNotNull(List *names, bool notNull) checkDomainOwner(tup); /* Is the domain already set to the desired constraint? */ - if (typTup->typnotnull == notNull) + if (!typTup->typnotnull && !notNull) { table_close(typrel, RowExclusiveLock); return address; } + if (typTup->typnotnull && notNull) + { + ScanKeyData key[1]; + SysScanDesc scan; + Relation pg_constraint; + Form_pg_constraint copy_con; + HeapTuple conTup; + HeapTuple copyTuple; + + pg_constraint = table_open(ConstraintRelationId, AccessShareLock); + + /* Look for NOT NULL Constraints on this domain */ + ScanKeyInit(&key[0], + Anum_pg_constraint_contypi
Re: ALTER TABLE ALTER CONSTRAINT misleading error message
On 2025-May-28, jian he wrote: > hi. > > create table t(a int, constraint cc check(a = 1)); > ALTER TABLE t ALTER CONSTRAINT cc not valid; > ERROR: FOREIGN KEY constraints cannot be marked NOT VALID > LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid; > ^ > > the error message seems misleading, We discussed this already, didn't we? There's a thread with IIRC three proposed patches for this. I think I liked this one the most: https://postgr.es/m/caaj_b97hd-jmts7ajgu6tdbczdx_kyukxg+k-dtymoieg+g...@mail.gmail.com -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Computing is too important to be left to men." (Karen Spärck Jones)
Re: Foreign key validation failure in 18beta1
Tender Wang 于2025年5月28日周三 18:54写道： > > > Antonin Houska 于2025年5月28日周三 15:51写道： > >> I've come across an unexpected ERROR during validation of FK constraint >> in PG >> 18beta1. The same works in PG 17: >> >> drop table if exists fk; >> drop table if exists pk; >> create table pk(i int primary key) partition by range (i); >> create table pk_1 partition of pk for values from (0) to (1); >> create table pk_2 partition of pk for values from (1) to (2); >> insert into pk values (0), (1); >> create table fk(i int); >> insert into fk values (1); >> >> -- Works if the FK constraint is created as valid. >> --alter table fk add foreign key(i) references pk; >> >> -- Fails if the FK constraint is created as NOT VALID and validated >> -- afterwards. >> alter table fk add foreign key(i) references pk not valid; >> alter table fk validate constraint fk_i_fkey; >> > > git bisect shows since below commit, the failure started. > > commit b663b9436e7509b5e73c8c372539f067cd6e66c1 > Author: Álvaro Herrera > Date: Thu Jan 23 15:54:38 2025 +0100 > > Allow NOT VALID foreign key constraints on partitioned tables > > I dided the codes, in QueueFKConstraintValidation(), we add three newconstraint for the fk rel, because the pk rel is partition table. During phase 3 of AlterTable, in ATRewriteTables(), call validateForeignKeyConstraint() three times. The first time the pk rel is pk, and it's ok. The second time the pk rel is only pk_1, and the type(1) is not in pk_1, so an error is reported. In this case, the two children newconstraint should not be added to the queue. -- Thanks, Tender Wang
Re: Foreign key validation failure in 18beta1
On 2025-May-28, Tender Wang wrote:
> I dided the codes, in QueueFKConstraintValidation(), we add three
> newconstraint for the
> fk rel, because the pk rel is partition table.
>
> During phase 3 of AlterTable, in ATRewriteTables(),
> call validateForeignKeyConstraint() three times.
> The first time the pk rel is pk, and it's ok.
> The second time the pk rel is only pk_1, and the type(1) is not in pk_1, so
> an error is reported.
>
> In this case, the two children newconstraint should not be added to the
> queue.
Yeah, I reached the same conclusion and this is the preliminary fix I
had written for it. I don't like that I had to duplicate a few lines of
code, but maybe it's not too bad. Also the comments need to be
clarified a bit more.
Clearly b663b9436e75 lacked tests.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Sallah, I said NO camels! That's FIVE camels; can't you count?"
(Indiana Jones)
>From 4c11da193991b95de8baf7689a9938ca9bb171aa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?=
Date: Wed, 28 May 2025 14:22:58 +0200
Subject: [PATCH] Don't validate referenced-side FK subconstraints
Silly bug in b663b9436e75.
Reported-by: Antonin Houska
Discussion: https://postgr.es/m/26983.1748418675@localhost
---
src/backend/commands/tablecmds.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 54ad38247aa..581c59f117b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12988,8 +12988,10 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel,
}
/*
- * If the table at either end of the constraint is partitioned, we need to
- * recurse and handle every constraint that is a child of this constraint.
+ * If the referencing table is partitioned, we need to recurse and handle
+ * every constraint that is a child of this constraint. Also update the
+ * sub-constraints on the referenced side of the constraint to appear as
+ * validated; no actual validation is needed for them.
*/
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
get_rel_relkind(con->confrelid) == RELKIND_PARTITIONED_TABLE)
@@ -13021,6 +13023,24 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel,
if (childcon->convalidated)
continue;
+ /*
+ * For child constraints in the referenced side of the constraint,
+ * we don't need to do any actual validation, but we do need to
+ * update the status to validated.
+ */
+ if (childcon->confrelid != con->confrelid)
+ {
+copyTuple = heap_copytuple(childtup);
+copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple);
+copy_con->convalidated = true;
+CatalogTupleUpdate(conrel, ©Tuple->t_self, copyTuple);
+
+InvokeObjectPostAlterHook(ConstraintRelationId, con->oid, 0);
+
+heap_freetuple(copyTuple);
+continue;
+ }
+
childrel = table_open(childcon->conrelid, lockmode);
QueueFKConstraintValidation(wqueue, conrel, childrel, childtup,
--
2.39.5
Re: Foreign key validation failure in 18beta1
Alvaro Herrera 于2025年5月28日周三 20:26写道：
> On 2025-May-28, Tender Wang wrote:
>
> > I dided the codes, in QueueFKConstraintValidation(), we add three
> > newconstraint for the
> > fk rel, because the pk rel is partition table.
> >
> > During phase 3 of AlterTable, in ATRewriteTables(),
> > call validateForeignKeyConstraint() three times.
> > The first time the pk rel is pk, and it's ok.
> > The second time the pk rel is only pk_1, and the type(1) is not in pk_1,
> so
> > an error is reported.
> >
> > In this case, the two children newconstraint should not be added to the
> > queue.
>
> Yeah, I reached the same conclusion and this is the preliminary fix I
> had written for it. I don't like that I had to duplicate a few lines of
> code, but maybe it's not too bad. Also the comments need to be
> clarified a bit more.
>
If the child table is still a partitioned table, the patch seems not work.
I figure out a quick fix as the attached. I add a bool argument into
the QueueFKConstraintValidation().
If it is true, it means we recursively call QueueFKConstraintValidation(),
then we don't add the newconstraint to the queue.
I'm not sure about this fix. Any thoughts?
--
Thanks,
Tender Wang
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 54ad38247aa..e11242e9b69 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -431,7 +431,7 @@ static ObjectAddress ATExecValidateConstraint(List **wqueue,
Relation rel, char *constrName,
bool recurse, bool recursing, LOCKMODE lockmode);
static void QueueFKConstraintValidation(List **wqueue, Relation conrel,
Relation rel,
-
HeapTuple contuple, LOCKMODE lockmode);
+
HeapTuple contuple, LOCKMODE lockmode, bool recursing);
static void QueueCheckConstraintValidation(List **wqueue, Relation conrel,
Relation rel,
char *constrName, HeapTuple contuple,
bool recurse, bool recursing, LOCKMODE lockmode);
@@ -11867,7 +11867,7 @@ AttachPartitionForeignKey(List **wqueue,
/* Use the same lock as for AT_ValidateConstraint */
QueueFKConstraintValidation(wqueue, conrel, partition,
partcontup,
-
ShareUpdateExclusiveLock);
+
ShareUpdateExclusiveLock, false);
ReleaseSysCache(partcontup);
table_close(conrel, RowExclusiveLock);
}
@@ -12919,7 +12919,7 @@ ATExecValidateConstraint(List **wqueue, Relation rel,
char *constrName,
{
if (con->contype == CONSTRAINT_FOREIGN)
{
- QueueFKConstraintValidation(wqueue, conrel, rel, tuple,
lockmode);
+ QueueFKConstraintValidation(wqueue, conrel, rel, tuple,
lockmode, false);
}
else if (con->contype == CONSTRAINT_CHECK)
{
@@ -12953,7 +12953,7 @@ ATExecValidateConstraint(List **wqueue, Relation rel,
char *constrName,
*/
static void
QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel,
- HeapTuple contuple,
LOCKMODE lockmode)
+ HeapTuple contuple,
LOCKMODE lockmode, bool recursing)
{
Form_pg_constraint con;
AlteredTableInfo *tab;
@@ -12983,8 +12983,11 @@ QueueFKConstraintValidation(List **wqueue, Relation
conrel, Relation rel,
newcon->qual = (Node *) fkconstraint;
/* Find or create work queue entry for this table */
- tab = ATGetQueueEntry(wqueue, rel);
- tab->constraints = lappend(tab->constraints, newcon);
+ if (!recursing)
+ {
+ tab = ATGetQueueEntry(wqueue, rel);
+ tab->constraints = lappend(tab->constraints, newcon);
+ }
}
/*
@@ -13024,7 +13027,7 @@ QueueFKConstraintValidation(List **wqueue, Relation
conrel, Relation rel,
childrel = table_open(childcon->conrelid, lockmode);
QueueFKConstraintValidation(wqueue, conrel, childrel,
childtup,
-
lockmode);
+
lockmode, true);
table_close(childrel, NoLock);
Re: fix: propagate M4 env variable to flex subprocess
On Wed, May 28, 2025 at 6:08 PM Andres Freund wrote: > Hi, > > On 2025-05-17 23:32:24 -0400, J. Javier Maestro wrote: > > On Tue, May 13, 2025 at 11:54 AM Andres Freund > wrote: > > > Bilal, I think you wrote this originally, do you recall? > > > > > > It seems like an issue beyond just M4... > > > > > > > IIRC the rest of the tools in the environment have ways to be specified > via > > Meson options (BISON, FLEX, PERL) so the only issue I see is Flex not > being > > able to find the specific m4 binary. What other issue(s) are you > > considering? > > PATH, LD_LIBRARY_PATH, ... > > I think this really should just add to the environment, rather than > supplant > it. Ah, understood. That definitely looks like a better option. > Do you want to write a patch like that? Otherwise I can. > Sure, I've attached the new patch. Let me know what you think, and if it's OK, what are the next steps to get the patch merged in main! Thanks, Javier 0001-fix-pgflex-propagate-environment-to-flex-subprocess.patch Description: Binary data
Re: PG 18 release notes draft committed
On 5/27/25 17:27, Bruce Momjian wrote: On Mon, May 26, 2025 at 10:20:08AM -0400, Joe Conway wrote: On 5/23/25 09:47, Bruce Momjian wrote: > On Fri, May 23, 2025 at 09:54:54AM +0200, Álvaro Herrera wrote: > > I also think that showing an XML-ish format of a commit message is > > unhelpful, not to mention hard to read. Showing a few actual examples > > would be better. > > Agreed, but the XML came from Joe Conway so I am hesitant to remove it > myself without feedback from him. I am not married to it, but I would say that I find pure examples confusing/ambiguous. To me at least the XML-ish specification is easier to understand. Perhaps both the specification as-is and one or two examples added? Yeah, I think some people like syntax, others like examples. What do you think about providing links into the archives for good representative log messages rather than making up a contrived example and/or copy/pasting them into the wiki page itself? Also, looking at the wiki page, my inclination would be to add an "Examples" section at the bottom of the wiki page -- does that work or do you think it ought to go just under the "General layout" syntax section? -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Expression push down from Join Node to below node.
Can you please resolve the doubt mentioned in the below thread? Thanks &Regards Shubhankar K Zlabs-CStore Member of Technical Staff, Zoho On Mon, 26 May 2025 16:58:12 +0530 Shubhankar Anand Kulkarni wrote --- Hi Andy, Thanks for the reply. As suggested, I tried using CTE which seems to solve the case. This is the updated plan: QUERY PLAN - Hash Right Join (cost=2500502930.90..2501167775.90 rows=1000 width=65) (actual time=1450.743..12200.977 rows=1000 loops=1) Output: t1.sensitive_data1, t2.column1 Hash Cond: (t2.column1 = t1.sensitive_data1) CTE t2 -> Foreign Scan on public.cipher (cost=0.00..2500163471.33 rows=1000 width=32) (actual time=0.095..6288.407 rows=1000 loops=1) Output: aes256_cbc_decrypt(cipher.c1, '\x323032356b6579666f726373746f726561657332353663626374657374696e67'::bytea, '\x323032356373746f7265697f7261657332353663626374657374696e67'::bytea) Read blocks: 2442 Skipped: 0 File Mode: File Per Table CStore Dir: /home/shubha-18887/Documents/zoho/postgres17/data/cstore_fdw/116838/116931 CStore Table Size: 2975 MB -> CTE Scan on t2 (cost=0.00..20.00 rows=1000 width=32) (actual time=0.097..7626.693 rows=1000 loops=1) Output: t2.column1 -> Hash (cost=136334.57..136334.57 rows=1000 width=33) (actual time=1448.676..1448.678 rows=1000 loops=1) Output: t1.sensitive_data1 Buckets: 131072 Batches: 128 Memory Usage: 5999kB -> Foreign Scan on public.benchmark_encytion t1 (cost=0.00..136334.57 rows=1000 width=33) (actual time=0.143..513.782 rows=1000 loops=1) Output: t1.sensitive_data1 Read blocks: 2442 Skipped: 0 File Mode: File Per Table CStore Dir: /home/shubha-18887/Documents/zoho/postgres17/data/cstore_fdw/116838/135230 CStore Table Size: 1987 MB Query Identifier: 2330480792947742169 Planning Time: 3.829 ms Execution Time: 12401.482 ms (24 rows) As seen, the expression got pushed down to respective foreign scan, reducing the overall query time reduced significantly, but there is an increase in the memory footprint. Also the thing which you mentioned, I guess the reason would be once we push the function down to the "foreign scan" node, we need to run these function *before any other filter*, which may increase the number of calls of the function. e.g. After looking into the code, it looks like we will first evaluate the qual, if it qualifies then only we will go for the projection. Meaning, even after expression push-down we will evaluate the expression on top filtered rows only. So I don't think it should have any concern in this case. Also for this particular case, I tried playing around create_hashjoin_plan function in createplan.c, to push my hash-clause expression to below foreign-scan. Currently, if there is a single hash clause of form funcExpr op Var, I am appending the funcExpr to pathTarget of respective table and replacing the funcExpr with a var. Is it a right place/approach to check or should we do changes while parsing in deconstruct_jointree(), in distiribute_quals_to_rels we can update the appropriate baserestrictinfo. Pls share your thoughts on the same. Thanks & Regards Shubhankar K ZLabs-CStore On Thu, 22 May 2025 14:21:51 +0530 Andy Fan < mailto:zhihuifan1...@163.com > wrote --- Shubhankar Anand Kulkarni < mailto:shubhankar...@zohocorp.com > writes: Hi, > SELECT sensitive_data1, column1 FROM benchmark_encytion AS t1 LEFT JOIN ( > SELECT aes256_cbc_decrypt( c1, '\x1234' :: > bytea, '\x5678' :: bytea ) AS column1 FROM cipher ) AS t2 ON > t1.sensitive_data1 = t2.column1; > > As you can see in the above Query, the join clause involves the column which > needs to be decrypted first and then joined > with other table and then in projection we need the needed decrypted values > to print as well, in this case the plan > generated by the PG is as mentioned below (refer to the image as well): > > > QUERY PLAN > > --- > > > Hash Right Join (cost=22.74..73.43 rows=1 width=65) >Output: t1.sensitive_data1, a
Re: PG 18 release notes draft committed
On Thu, 1 May 2025 22:44:50 -0400 Bruce Momjian wrote: > I have committd the first draft of the PG 18 release notes. The item > count looks strong: Some items in the "EXPLAIN" section are actually not about the EXPLAIN but the ANALYZE command. The attached patch is move them to the "Utility Commands" section with a little edit of wordings. Regards, Yugo Nagata -- Yugo Nagata diff --git a/doc/src/sgml/release-18.sgml b/doc/src/sgml/release-18.sgml index 2ae03065f94..0008c9a9f75 100644 --- a/doc/src/sgml/release-18.sgml +++ b/doc/src/sgml/release-18.sgml @@ -1479,6 +1479,33 @@ This is enabled with the new ONLY option. This is useful since autovacuum does + + + + +Add WAL, CPU, and average read statistics output to ANALYZE VERBOSE (Anthonin Bonnefoy) +§ +§ + + + + + + + +Add full WAL buffer count to VACUUM/ANALYZE (VERBOSE) and autovacuum log output (Bertrand Drouvot) +§ + + + - - - -Add WAL, CPU, and average read statistics output to EXPLAIN ANALYZE VERBOSE (Anthonin Bonnefoy) -§ -§ - - - -Add full WAL buffer count to EXPLAIN (WAL), VACUUM/ANALYZE (VERBOSE), and autovacuum log output (Bertrand Drouvot) +Add full WAL buffer count to EXPLAIN (WAL) output (Bertrand Drouvot) § -§
Re: fix: propagate M4 env variable to flex subprocess
On Tue, May 20, 2025 at 8:53 AM Nazir Bilal Yavuz wrote: > Hi, > > On Tue, 13 May 2025 at 18:54, Andres Freund wrote: > > > > Hi, > > > > On 2025-05-12 23:14:59 -0400, J. Javier Maestro wrote: > > > The pgflex wrapper runs flex with an explicit environment, so it > doesn't > > > inherit environment variables from the parent process. However, flex > can > > > use the M4 env variable and/or the PATH (via execvp) to find the m4 > macro > > > processor. > > > > > Thus, since flex honors the M4 env variable, it should be propagated > to the > > > subprocess environment if it's set in the parent environment. This > patch > > > fixes it. > > > > I think it probably was not intentional to fully specify the environment, > > rather than just *adding* FLEX_TMP_DIR to the caller's environment. > Bilal, I > > think you wrote this originally, do you recall? > > I found the original pull request [1] but it does not include the > 'FLEX_TMP_DIR' part. I tried to search where the 'FLEX_TMP_DIR' part > is added [2], it seems that this part is added while rebasing so there > is no specific commit. > > [1] https://github.com/anarazel/postgres/pull/51 > [2] > https://github.com/anarazel/postgres/commit/cd817afd4ab006b90307a940d96b5116d649165c Thanks for the context. Apart from these pointers to the original PRs, could you (and Andres) give me feedback on the patch? Do you think it's OK to merge? Should I add it to a commitfest? On this note, on top of that patch, I also have other patches that I think would be relevant. Given that you and Andres seem to use Github, here are the patches that I would like to discuss: https://github.com/monogres/postgres/compare/REL_16_0...monogres/patches/16.0 The last one is more of a "hack" but still, it shows the issues with the current approach to "embedding metadata" in header files that end up in the binaries. What would be the most effective way to discuss these patches? Would it be better to go one-by-one or to create a new thread focused on discussing reproducibility, to discuss all of them? Thanks a lot beforehand for your help, Regards, Javier
Re: wrong query results on bf leafhopper
Robins Tharakan writes: > I didn't dive in deeper but I see that indri failed recently [1] on what > seems > like the exact same test / line-number (at t/027_stream_regress.pl line 95) > that leafhopper has been tripping on recently. The error is not verbatim, > but it was a little too coincidental to not highlight here. 027_stream_regress.pl is quite a large/complicated test, and for reasons that are not clear to me it seems more prone to intermittent timing problems than most other tests. I would not read very much into that being the test that failed for you, especially since the detailed symptoms are not like indri's. regards, tom lane
Re: pg16 && GSSAPI && Heimdal/Macos
"Todd M. Kover" writes: > Wehere did this end up getting decided? I'm hoping, if it's going to > make it into main/master, it will be able to also make it's way ingo pg18. I don't think anything's been decided. I've expressed my opinion, but I'm just one person. I'd hoped some other people who are interested in Postgres security matters would comment. Even granting that we're okay with letting people build against Heimdal, I'm not clear on the path forward. Your patch proposes to effectively disable gss_accept_delegation, which isn't real palatable (and would require docs and test fixes that aren't there). Nico seemed to think that there is a way to perform delegation without using gss_store_cred_into; if we could avoid that loss of functionality, it'd go a long way towards making the idea more acceptable. I also wonder about whether we ought to try to use GSS.framework on Mac. I can say though that it's definitively too late for v18; we've been in feature freeze for months. regards, tom lane
Re: Add CHECK_FOR_INTERRUPTS in polling loop code path in XactLockTableWait
Hi Fujii, The WaitEvent sounds good to me, I will submit a separate patch for that when I find time. Kevin On Wed, May 28, 2025 at 5:06 PM Fujii Masao wrote: > > > On 2025/05/27 4:43, Kevin K Biju wrote: > > Hi Fujii, > > > > Thanks for the review. > > So unless there are any objections, I'm planning to commit > the patch with the following commit message. > > --- > Make XactLockTableWait() and ConditionalXactLockTableWait() interruptable > more. > > Previously, XactLockTableWait() and ConditionalXactLockTableWait() could > enter > a non-interruptible loop when they successfully acquired a lock on a > transaction > but the transaction still appeared to be running. Since this loop continued > until the transaction completed, it could result in long, uninterruptible > waits. > > Although this scenario is generally unlikely since a transaction lock is > basically acquired only when the transaction is not running, it can occur > in a hot standby. In such cases, the transaction may still appear active > due to > the KnownAssignedXids list, even while no lock on the transaction exists. > For example, this situation can happen when creating a logical replication > slot > on a standby. > > The cause of the non-interruptible loop was the absence of > CHECK_FOR_INTERRUPTS() > within it. This commit adds CHECK_FOR_INTERRUPTS() to the loop in both > functions, > ensuring they can be interrupted safely. > > Back-patch to all supported branches. > > Author: Kevin K Biju > Reviewed-by: Fujii Masao > Discussion: > https://postgr.es/m/CAM45KeELdjhS-rGuvN=zlj_asvzacucz9lzwvzh7bgcd12d...@mail.gmail.com > Backpatch-through: 13 > --- > > > > > Just an idea: how about calling pgstat_report_wait_start() and _end() > around pg_usleep(), similar to what WaitExceedsMaxStandbyDelay() does? > > > > I was thinking about this as well, but the question is what do we report > the wait event here as? The one that makes sense to me is PG_WAIT_LOCK, but > that wouldn't align with what pg_locks would display since there is no > actual lock grabbed on the standby. > > I couldn't find an existing wait event that fits this case, so how about > adding a new one, like IPC/XactDone, to indicate "Waiting for the > transaction > to commit or abort"? > > > > > Also, would waiting only 1ms per loop cycle be too aggressive? > > > > I agree that it's aggressive for this case. But > XactLockTableWait/ConditionalXactLockTableWait seem to be used in other > places including in heapam so I'm hesitant on changing this behaviour for > all of them. > I agree that we need more time to consider whether this change is safe. > > Regards, > > -- > Fujii Masao > NTT DATA Japan Corporation > >
Re: Fixing memory leaks in postgres_fdw
I wrote:
> Having said that, the idea that this sequence is OOM-safe is pretty
> silly anyway, considering that createNewConnection does a pstrdup,
> and creates a new hashtable entry which might require enlarging the
> hashtable, and for that matter might even create the hashtable.
> So maybe rather than continuing to adhere to a half-baked coding
> rule, we need to think of some other way to do that.
Here's an attempt at fixing this properly. I'm posting it as a
standalone patch because I now think this part might be worth
back-patching. The odds of an OOM at just the wrong time aren't
high, but losing track of an open connection seems pretty bad.
regards, tom lane
From 1d32ebcbae932c3d473fe90094e3c4c90723c0eb Mon Sep 17 00:00:00 2001
From: Tom Lane
Date: Wed, 28 May 2025 16:52:20 -0400
Subject: [PATCH v5] Avoid resource leaks when a dblink connection fails.
If we hit out-of-memory between creating the PGconn and inserting
it into dblink's hashtable, we'd lose track of the PGconn, which
is quite bad since it represents a live connection to a remote DB.
Fix by rearranging things so that we create the hashtable entry
first.
Also reduce the number of states we have to deal with by getting rid
of the separately-allocated remoteConn object, instead allocating it
in-line in the hashtable entries. (That incidentally removes a
session-lifespan memory leak observed in the regression tests.)
There is an apparently-irreducible remaining OOM hazard, which
is that if the connection fails at the libpq level (ie it's
CONNECTION_BAD) then we have to pstrdup the PGconn's error message
before we can release it, and theoretically that could fail. However,
in such cases we're only leaking memory not a live remote connection,
so I'm not convinced that it's worth sweating over.
Author: Tom Lane
Discussion: https://postgr.es/m/1346940.1748381...@sss.pgh.pa.us
---
contrib/dblink/dblink.c | 78 ++---
1 file changed, 42 insertions(+), 36 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 98d4e3d7dac..8a0b112a7ff 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -105,7 +105,7 @@ static PGresult *storeQueryResult(volatile storeInfo *sinfo, PGconn *conn, const
static void storeRow(volatile storeInfo *sinfo, PGresult *res, bool first);
static remoteConn *getConnectionByName(const char *name);
static HTAB *createConnHash(void);
-static void createNewConnection(const char *name, remoteConn *rconn);
+static remoteConn *createNewConnection(const char *name);
static void deleteConnection(const char *name);
static char **get_pkey_attnames(Relation rel, int16 *indnkeyatts);
static char **get_text_array_contents(ArrayType *array, int *numitems);
@@ -119,7 +119,8 @@ static Relation get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclM
static char *generate_relation_name(Relation rel);
static void dblink_connstr_check(const char *connstr);
static bool dblink_connstr_has_pw(const char *connstr);
-static void dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr);
+static void dblink_security_check(PGconn *conn, const char *connname,
+ const char *connstr);
static void dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
bool fail, const char *fmt,...) pg_attribute_printf(5, 6);
static char *get_connect_string(const char *servername);
@@ -147,16 +148,22 @@ static uint32 dblink_we_get_conn = 0;
static uint32 dblink_we_get_result = 0;
/*
- * Following is list that holds multiple remote connections.
+ * Following is hash that holds multiple remote connections.
* Calling convention of each dblink function changes to accept
- * connection name as the first parameter. The connection list is
+ * connection name as the first parameter. The connection hash is
* much like ecpg e.g. a mapping between a name and a PGconn object.
+ *
+ * To avoid potentially leaking a PGconn object in case of out-of-memory
+ * errors, we first create the hash entry, then open the PGconn.
+ * Hence, a hash entry whose rconn.conn pointer is NULL must be
+ * understood as a leftover from a failed create; it should be ignored
+ * by lookup operations, and silently replaced by create operations.
*/
typedef struct remoteConnHashEnt
{
char name[NAMEDATALEN];
- remoteConn *rconn;
+ remoteConn rconn;
} remoteConnHashEnt;
/* initial number of connection hashes */
@@ -233,7 +240,7 @@ dblink_get_conn(char *conname_or_str,
errmsg("could not establish connection"),
errdetail_internal("%s", msg)));
}
- dblink_security_check(conn, rconn, connstr);
+ dblink_security_check(conn, NULL, connstr);
if (PQclientEncoding(conn) != GetDatabaseEncoding())
PQsetClientEncoding(conn, GetDatabaseEncodingName());
freeconn = true;
@@ -296,15 +303,6 @@ dblink_connect(PG_FUNCTION_ARGS)
else if (PG_NARGS() == 1)
conname_or_str = text_to_cstring(PG_GETARG_TEXT
Re: queryId constant squashing does not support prepared statements
>> * 0001:
> You have mentioned the addition of tests, but v6-0001 includes nothing
> of the kind. Am I missing something? How much coverage did you
> intend to add here? These seem to be included in squashing.sql in
> patch v6-0002, but IMO this should be moved somewhere else to work
> with the back-branches and make the whole backpatch story more
> consistent.
That's my mistake. I added a new file called normalize.sql to test
specific normalization scenarios. Added in v7
> > * 0002:
> RelabelType perhaps?
Fixed.
> A lot of the tests introduced in v6-0002 are copy-pastes of the
> previous ones for IN clauses introduced for the ARRAY cases, with
> comments explaining the reasons why lists are squashed or not also
> copy-pasted. Perhaps it would make sense to group the ARRAY and IN
> clause cases together. For example, group each of the two CoerceViaIO
> cases together in a single query on pg_stat_statements, with a single
> pg_stat_statements_reset(). That would make more difficult to miss
> the fact that we need to care about IN clauses *and* arrays when
> adding more test patterns, if we add some of course.
>
I agree. I reorganized by grouping both for IN and ARRAY tests
together for a specific test area.
I also clarified some comments in the tests, etc.
> > * 0003:
> I'd suggest to not use "n" for this one, but a different variable
> name, leaving the internals for the SubLink cases minimally touched.
I agree. Fixed.
> Implementation-wise, I would choose a location with a query length
> rather than start and end locations. That's what we do for the nested
> queries in the DMLs, so on consistency grounds..
This is different because the existing location field is tracking
something a bit different than what we want to track.
What the current location field is tracking is to assist in things
like error messages, like below, which wants to place the
caret (^) in the proper location, which is at the location of the
"IN".
```
ERROR: operator does not exist: oid = text
LINE 1: select where 1::oid IN (1::text, 2, 3);
^
HINT: No operator matches the given name and argument types. You
might need to add explicit type casts.
test=#
```
What we need for squashing is to track the start of the outer '(' and ')' of
the expression.
I could do something like fields to track list_start and list_length instead,
Will that be better to be closer in consistency?
> > * 0004: implements external parameter squashing.
> Using a custom implementation for Param nodes means that we are going
> to apply a location record for all external parameters, not only the
> ones in the lists.. Not sure if this is a good idea. Something
> smells a bit wrong with this approach. Sorry, I cannot push my finger
> on what exactly when typing this paragraph.
Actually, only the parameters outside of the squashed lists are
recorded. I added
a comment to make that clear. I would really want to only record parameter
locations if we know we have a squashed list, but it's impossible to
know that in
advance.
Also, the reason for a custom implementation for Param is to avoid having
to change the signature of JUMBLE_LOCATION because we have a
new bool argument to RecordExpressionLocation to set a location as an
external parameter. We will also need special handling in gen_node_support.pl
for Param to set the new argument. I was not too happy with doing that.
> Patches 0002 and 0003 fix bugs in the squashing logic present only on
> HEAD, nothing that impacts older branches already released, right?
That is correct.
--
Sami
v7-0003-Fix-Normalization-for-squashed-query-texts.patch
Description: Binary data
v7-0001-Fix-broken-normalization-due-to-duplicate-constan.patch
Description: Binary data
v7-0002-Enhanced-query-jumbling-squashing-tests.patch
Description: Binary data
v7-0004-Support-Squashing-of-External-Parameters.patch
Description: Binary data
Re: pg16 && GSSAPI && Heimdal/Macos
On Wed, May 28, 2025 at 8:53 AM Tom Lane wrote: > Even granting that we're okay with letting people build against > Heimdal, I'm not clear on the path forward. Your patch proposes > to effectively disable gss_accept_delegation, which isn't real > palatable (and would require docs and test fixes that aren't there). > Nico seemed to think that there is a way to perform delegation > without using gss_store_cred_into; if we could avoid that loss of > functionality, it'd go a long way towards making the idea more > acceptable. I also wonder about whether we ought to try to use > GSS.framework on Mac. Personally, I'd be more happy to "maintain GSS on Mac using non-deprecated interfaces" than "maintain GSS via Heimdal, best-effort, some of the time". I think the former puts less of a burden on our testing matrix. --Jacob
Re: PG 18 release notes draft committed
On Wed, May 28, 2025 at 10:55:50AM +0100, Dean Rasheed wrote: > For "Improve the speed of multiplication", I think it should say > "numeric multiplication" rather than simply "multiplication", and I > think it's worth also linking to commits ca481d3c9ab and c4e44224cf6 > which were part of the same work. > > I think it's also worth mentioning 9428c001f67, which sped up numeric > division. That can be included in the same item, as in the attached > patch, unless you think it's worth listing it as a separate item. Great, patch applied. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Do not let urgent matters crowd out time for investment in the future.
Re: wrong query results on bf leafhopper
Hi, On 2025-05-28 22:51:14 +0930, Robins Tharakan wrote: > On Tue, 20 May 2025 at 15:20, David Rowley wrote: > > > On Tue, 20 May 2025 at 16:07, Tom Lane wrote: > > > Failures like this one [1]: > > > > > > @@ -340,9 +340,13 @@ > > > create function myinthash(myint) returns integer strict immutable > > language > > >internal as 'hashint4'; > > > NOTICE: argument type myint is only a shell > > > +ERROR: ROWS is not applicable when function does not return a set > > > > > > are hard to explain as anything besides "that machine is quite > > > broken". Whether it's flaky hardware, broken compiler, or what is > > > undeterminable from here, but I don't believe it's our bug. So I'm > > > unexcited about putting effort into it. > > > > There are certainly much fewer moving parts in PostgreSQL code for > > that one as this failure doesn't seem to rely on anything stored in > > any tables or the catalogues. > > > > I'd have thought it would be unlikely to be a compiler bug as wouldn't > > that mean it'd fail every time? > > > > > Recently leafhopper failed again on the same test. For now I've paused it. > To rule out the compiler (and its maturity on the architecture), I'll > upgrade > gcc (to nightly, or something more recent) and then re-enable to see if it > changes anything. +1 to a gcc upgrade, gcc 11 is rather old and out of upstream support. A kernel upgrade would be good too. My completely baseless gut feeling is that some SIMD registers occassionally get corrupted, e.g. due to a kernel interrupt / context switch not properly storing & restoring them. Weirdly enought the instrumentation code is among the pieces of PG code most vulnerable to that because we mostly don't do enough auto-vectorizable math, but InstrEndLoop(), InstrStopNode() etc are trivially auto-vectorizable. I'm pretty sure I've previously analyzed problems around this, but don't remember the details (IA64 maybe?). > I didn't dive in deeper but I see that indri failed recently [1] on what > seems > like the exact same test / line-number (at t/027_stream_regress.pl line 95) > that leafhopper has been tripping on recently. The error is not verbatim, > but it was a little too coincidental to not highlight here. For 027_stream_regress.pl you really need to look at regress_log_027_stream_regress.log, as that specific line just tests whether the standard regression tests passed. The failure on indri is rather different than your issue, I doubt there's an overlap between the problems... I think we should spruce up 027_stream_regress.pl a bit around this. Before the "regression tests pass" check we should a) check if primary is still alive b) check if standby is still alive and then, iff a) & b) pass, in addition to printing the entire regression test file, we should add the head and tail of regression.diffs to the failure message, so one can quickly glean what went wrong. Greetings, Andres Freund
Re: fix: propagate M4 env variable to flex subprocess
Hi, On 2025-05-17 23:32:24 -0400, J. Javier Maestro wrote: > On Tue, May 13, 2025 at 11:54 AM Andres Freund wrote: > > Bilal, I think you wrote this originally, do you recall? > > > > It seems like an issue beyond just M4... > > > > IIRC the rest of the tools in the environment have ways to be specified via > Meson options (BISON, FLEX, PERL) so the only issue I see is Flex not being > able to find the specific m4 binary. What other issue(s) are you > considering? PATH, LD_LIBRARY_PATH, ... I think this really should just add to the environment, rather than supplant it. Do you want to write a patch like that? Otherwise I can. Greetings, Andres Freund
Re: wrong query results on bf leafhopper
On Tue, 20 May 2025 at 15:20, David Rowley wrote: > On Tue, 20 May 2025 at 16:07, Tom Lane wrote: > > Failures like this one [1]: > > > > @@ -340,9 +340,13 @@ > > create function myinthash(myint) returns integer strict immutable > language > >internal as 'hashint4'; > > NOTICE: argument type myint is only a shell > > +ERROR: ROWS is not applicable when function does not return a set > > > > are hard to explain as anything besides "that machine is quite > > broken". Whether it's flaky hardware, broken compiler, or what is > > undeterminable from here, but I don't believe it's our bug. So I'm > > unexcited about putting effort into it. > > There are certainly much fewer moving parts in PostgreSQL code for > that one as this failure doesn't seem to rely on anything stored in > any tables or the catalogues. > > I'd have thought it would be unlikely to be a compiler bug as wouldn't > that mean it'd fail every time? > Recently leafhopper failed again on the same test. For now I've paused it. To rule out the compiler (and its maturity on the architecture), I'll upgrade gcc (to nightly, or something more recent) and then re-enable to see if it changes anything. I didn't dive in deeper but I see that indri failed recently [1] on what seems like the exact same test / line-number (at t/027_stream_regress.pl line 95) that leafhopper has been tripping on recently. The error is not verbatim, but it was a little too coincidental to not highlight here. - robins Ref: 1. https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=indri&dt=2025-05-23%2020%3A30%3A07
Re: pg16 && GSSAPI && Heimdal/Macos
Wehere did this end up getting decided? I'm hoping, if it's going to make it into main/master, it will be able to also make it's way ingo pg18. As Nico wrote, my interest in opening this was especially for Apple's implementation of Kerberos (and to a lesser extent NetBSD and FreeBSD)'s which, today, happens to be based on Heimdal. Additionally, the patch I put out there, at Tom's suggestion, just makes build-time decisions and isn't Heimdal specific, per say, just happens to land that way now. Thanks, -Todd > On Tue, Apr 08, 2025 at 02:18:40AM -0400, Tom Lane wrote: > > Nico Williams writes: > > > Heimdal in the master branch sure does; I'm the author if > > > gss_store_cred_into() and gss_store_cred_into2(). Idk when we'll do an > > > 8.0 release though. We've run out of steam. > > > > Yeah, this is what makes me fearful about putting in changes to re-support > > Heimdal. It seems like it's more or less abandonware so far as the > > upstream developers are concerned, which is not comforting with any > > package, but especially not for security-critical code. I understand > > Right now heimdal/heimdal basically is abandonware, yes, but not > Apple's. I wish it weren't so as I've had a great deal of fun working > on it over the past decade plus, but there's loads of other things to > work on. > > > that downstream packagers such as Apple and the BSDen are trying to > > fill the gap, but how much should their efforts be relied on? > > > > We could certainly take the attitude suggested upthread that > > "we'll allow you to build with Heimdal, and if it breaks you > > get to keep both pieces". But I dunno. We get blamed when > > users do obviously-stupid stuff like use a guessable superuser > > password on a database they've exposed to the internet [eg, 1]. > > It's the _GSS-API_. It's pretty much a standard. You're making use of > a non-standard extension (gss_store_cred_into()) that's not strictly > needed since you could just either a) keep the gss_cred_id_t value > output by gss_accept_sec_context() and use that if needed anywhere > (probably only postgres_fdw), or b) not accept delegated credentials. > (a) might require changes to the interface to extensions, idk. But > anyways, if you were not using non-standard extensions then you wouldn't > need to care if the user wants to link postgres with Heimdal or MIT or > anything else. Heck, there's a GSS wrapper around SSPI that can be used > on Windows even (though it's obscure). > > gss_store_cred() is a standards-track RFC, though it turns out to not be > good enough, which is why gss_store_cred_into() and > gss_store_cred_into2() exist, but in this particular case (PG) > gss_store_cred() is in fact good enough (see other reply). > > > It would be a lot more obviously our fault if we say nothing > > when a user chooses a known-insecure library to build against. > > Sure. > > > So I've still got really mixed emotions about this project. > > I totally understand the desire to use these library versions, > > but I can't help fearing that people will regret doing so ... > > and then want to shift the blame to us. > > In this case OP wants to use _Apple_'s Heimdal so they can make sure > they're using Apple's API: ccache. Apple presumably supports their > Heimdal fork even if the upstream is currently abandonware-ish. I think > you should always allow the use of the OS's authn. infrastructure where > you have the code for it. > > Nico > -- >
Re: Logical Replication of sequences
On Thu, May 22, 2025 at 10:42 PM vignesh C wrote:
>
>
> The attached v20250522 patch has the changes for the same.
>
Thank you for the patches, please find comments for patch-0004.
1)
+/*
+ * report_error_sequences
+ *
+ * Logs a warning listing all sequences that are missing on the publisher,
+ * as well as those with value mismatches relative to the subscriber.
+ */
+static void
+report_error_sequences(StringInfo missing_seqs, StringInfo mismatched_seqs)
The function description should be updated to reflect the recent
changes, as it now raises an error instead of issuing a warning.
2)
+ ereport(ERROR, errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("%s", combined_error_msg->data));
+
+ destroyStringInfo(combined_error_msg);
+}
I think we can remove destroyStringInfo() as we will never reach here
in case of error.
3)
+ * we want to avoid keeping this batch transaction open for extended
+ * periods so it iscurrently limited to 100 sequences per batch.
+ */
typo : iscurrently / is currently
4)
+ HeapTuple tup;
+ Form_pg_sequence seqform;
+ LogicalRepSequenceInfo *seqinfo;
+
[...]
+ Assert(seqinfo);
Since there's an assertion for 'seqinfo', it would be safer to
initialize it to NULL to avoid any unexpected behavior.
6)
+ if (missing_seqs->len || mismatched_seqs->len)
+ report_error_sequences(missing_seqs, mismatched_seqs);
I think it would be helpful to add a comment for this check, perhaps
something like:
/*
* Report an error if any sequences are missing on the remote side
* or if local sequence parameters don't match with the remote ones.
*/
Please rephrase if needed.
--
Thanks,
Nisha
Re: [PING] fallocate() causes btrfs to never compress postgresql files
On 5/28/25 16:22, Dimitrios Apostolou wrote: > Hello, sorry for mass sending this, but I didn't get any response to my > first email [1] so I'm now CC'ing the commit's 4d330a6 [2] author and > the reviewers. I think it's an important issue, because I need to > custom-compile postgresql to have what I had before: a transparently > compressed database. > That message arrived a couple days before the feature freeze, so everyone was busy with getting PG18 patches over the line. I assume that's why no one responded to a message about an issue that already affects PG17. We're in the quieter part of the dev cycle, people are recovering etc. Hence the delay. > [1] https://www.postgresql.org/message-id/d0f4fc11-969d-7b3a- > aacf-00f86450e...@gmx.net > [2] https://github.com/postgres/postgres/ > commit/4d330a61bb1969df31f2cebfe1ba9d1d004346d8 > > My previous message follows: > > Hi, > > this is just a heads-up about files being generated by PostgreSQL 17 not > being compressed by Btrfs, even when mounted with the force-compress mount > option. I have this occuring aggressively when restoring a database via > pg_restore. I think this is caused mdzeroextend() calling FileFallocate(), > which in turn invokes posix_fallocate(). > Right, I don't think we're really using posix_fallocate() in other places, or at least not in places that would matter. And this code comes from commit 4d330a61bb in PG17: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4d330a61bb1969df31f2cebfe1ba9d1d004346d8 The commit message explains why we do that - it has advantages when allocating large number of blocks. FWIW it's a general code, when we need to add space to a relation, not just for pg_restore. > I also verified that turning off the use of fallocate causes the database > to write compressed files again, like it did in older versions. > Unfortunately the only way I found was to configure with a "hack" so that > autoconf thinks the feature is not available: > > ./configure ac_cv_func_posix_fallocate=no > Unfortunately, that seems pretty heavy handed, because it will affect the whole build, no matter which filesystem it gets used with. And I guess we don't want to disable posix_fallocate() just because one filesystem does something ... strange. > There have been discussions on the btrfs mailing list about why it does > that, the summary is that it is very difficult to guarantee that > compressed writes will not fail with ENOSPACE on a CoW filesystem, thus > files with fallocate()d ranges are treated as being marked NOCOW, > effectively disabling compression. > Isn't guaranteeing success of a write a general issue with compressed filesystem? Why is posix_fallocate() any special in this regard? Shouldn't the filesystem be defensive and assume the data is not compressible? Or maybe just return EOPNOTSUPP when in doubt. > Should PostgreSQL provide a setting to avoid the use of fallocate()? Or is > it the filesystem at fault for not returning EOPNOTSUPP, in which case > postgres would use its fallback code? > I don't have a clear opinion on whether it's a filesystem issue. Maybe we should be handling this differently, not sure. > BTW even in the last case, PostgreSQL would not notice the lack of > fallocate() support as glibc implements a userspace fallback in > posix_fallocate(). That fallback has its own issues that hopefully will > not affect postgres (see CAVEATS in man 3 posix_fallocate). > Well, if btrfs starts returning EOPNOTSUPP, and glibc switches to the userspace fallback, we wouldn't notice. But that's up to the btrfs to decide if they want to support fallocate. We still need our fallback anyway, because of other OSes. regards -- Tomas Vondra
[PING] fallocate() causes btrfs to never compress postgresql files
Hello, sorry for mass sending this, but I didn't get any response to my first email [1] so I'm now CC'ing the commit's 4d330a6 [2] author and the reviewers. I think it's an important issue, because I need to custom-compile postgresql to have what I had before: a transparently compressed database. [1] https://www.postgresql.org/message-id/d0f4fc11-969d-7b3a-aacf-00f86450e...@gmx.net [2] https://github.com/postgres/postgres/commit/4d330a61bb1969df31f2cebfe1ba9d1d004346d8 My previous message follows: Hi, this is just a heads-up about files being generated by PostgreSQL 17 not being compressed by Btrfs, even when mounted with the force-compress mount option. I have this occuring aggressively when restoring a database via pg_restore. I think this is caused mdzeroextend() calling FileFallocate(), which in turn invokes posix_fallocate(). I also verified that turning off the use of fallocate causes the database to write compressed files again, like it did in older versions. Unfortunately the only way I found was to configure with a "hack" so that autoconf thinks the feature is not available: ./configure ac_cv_func_posix_fallocate=no There have been discussions on the btrfs mailing list about why it does that, the summary is that it is very difficult to guarantee that compressed writes will not fail with ENOSPACE on a CoW filesystem, thus files with fallocate()d ranges are treated as being marked NOCOW, effectively disabling compression. Should PostgreSQL provide a setting to avoid the use of fallocate()? Or is it the filesystem at fault for not returning EOPNOTSUPP, in which case postgres would use its fallback code? BTW even in the last case, PostgreSQL would not notice the lack of fallocate() support as glibc implements a userspace fallback in posix_fallocate(). That fallback has its own issues that hopefully will not affect postgres (see CAVEATS in man 3 posix_fallocate). Regards, Dimitris
Re: PG 18 release notes draft committed
On Wed, 28 May 2025 23:14:36 +0900 Yugo Nagata wrote: > On Thu, 1 May 2025 22:44:50 -0400 > Bruce Momjian wrote: > > > I have committd the first draft of the PG 18 release notes. The item > > count looks strong: > > Some items in the "EXPLAIN" section are actually not about the EXPLAIN > but the ANALYZE command. The attached patch is move them to the > "Utility Commands" section with a little edit of wordings. ... or, perhaps they should be moved to the "Monitoring" section. I've attached a patch in this version. Regards Yugo Nagata -- Yugo Nagata diff --git a/doc/src/sgml/release-18.sgml b/doc/src/sgml/release-18.sgml index 2ae03065f94..7cd350a973a 100644 --- a/doc/src/sgml/release-18.sgml +++ b/doc/src/sgml/release-18.sgml @@ -761,6 +761,33 @@ mode; tracking must be enabled with the server variable track_cost_delay_timing. + + + + +Add WAL, CPU, and average read statistics output to ANALYZE VERBOSE (Anthonin Bonnefoy) +§ +§ + + + + + + + +Add full WAL buffer count to VACUUM/ANALYZE (VERBOSE) and autovacuum log output (Bertrand Drouvot) +§ + + + - - - -Add WAL, CPU, and average read statistics output to EXPLAIN ANALYZE VERBOSE (Anthonin Bonnefoy) -§ -§ - - - -Add full WAL buffer count to EXPLAIN (WAL), VACUUM/ANALYZE (VERBOSE), and autovacuum log output (Bertrand Drouvot) +Add full WAL buffer count to EXPLAIN (WAL) output (Bertrand Drouvot) § -§
Re: pg16 && GSSAPI && Heimdal/Macos
Jacob Champion writes:
> - I also want to draw attention to the fact that libpq can't claim
> that a credential is delegated if it's not; that breaks the security
> of our FDWs. So pg_store_delegated_credential() cannot be a no-op.
Right. What I had in mind if we cannot find an alternative
implementation was
void
pg_store_delegated_credential(gss_cred_id_t cred)
{
#ifdef HAVE_GSS_STORE_CRED_INTO
...
major = gss_store_cred_into(&minor,
...
#else
elog(ERROR, "credential delegation is not implemented");
#endif
}
combined with a check_hook that prevents the gss_accept_delegation
GUC from being set to "true" if not HAVE_GSS_STORE_CRED_INTO.
(That should make the above-depicted elog unreachable, but
belt and suspenders too isn't a bad plan.)
regards, tom lane
Re: pg16 && GSSAPI && Heimdal/Macos
On Wed, May 28, 2025 at 2:59 PM Tom Lane wrote: > (That should make the above-depicted elog unreachable, but > belt and suspenders too isn't a bad plan.) I like that approach, if delegation on Mac ends up being too much of a pain. --Jacob
Re: Standardize the definition of the subtype field of AlterDomainStmt
Updated
On 2025/5/28 19:30, Tender Wang wrote:
Peter Eisentraut mailto:pe...@eisentraut.org>> 于
2025年5月28日周三 19:23写道：
On 27.05.25 05:06, Quan Zongliang wrote:
> I noticed that the subtype of AlterDomainStmt is directly using
> constants in the code. It is not conducive to the maintenance and
> reading of the code. Based on the definition of AlterTableType, use
> "AD_" as the prefix. Define several macros to replace the original
> characters.
> The subtype of AlterTableCmd is defined using an enumeration. The
> subtypes of AlterDomainStmt are relatively few in number, and the
> original definition uses characters. These definitions still use
> characters and maintain the values unchanged. If some plugins or
tools
> are also processing AlterDomainStmt, there will be no errors.
You can still make it an enum and assign the currently in use values to
the new symbols, like
enum AlterDomainType
{
AD_AlterDefault = 'T',
AD_DropNotNull = 'N',
...
I would prefer that.
+1
--
Thanks,
Tender Wang
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 54ad38247aa..3f09f85a480 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15696,7 +15696,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid
refRelId, char *cmd,
{
AlterDomainStmt *stmt = (AlterDomainStmt *) stm;
- if (stmt->subtype == 'C') /* ADD CONSTRAINT */
+ if (stmt->subtype == AD_AddConstraint)
{
Constraint *con = castNode(Constraint,
stmt->def);
AlterTableCmd *cmd = makeNode(AlterTableCmd);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0b5652071d1..103021c0947 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -11629,7 +11629,7 @@ AlterDomainStmt:
{
AlterDomainStmt *n =
makeNode(AlterDomainStmt);
- n->subtype = 'T';
+ n->subtype = AD_AlterDefault;
n->typeName = $3;
n->def = $4;
$$ = (Node *) n;
@@ -11639,7 +11639,7 @@ AlterDomainStmt:
{
AlterDomainStmt *n =
makeNode(AlterDomainStmt);
- n->subtype = 'N';
+ n->subtype = AD_DropNotNull;
n->typeName = $3;
$$ = (Node *) n;
}
@@ -11648,7 +11648,7 @@ AlterDomainStmt:
{
AlterDomainStmt *n =
makeNode(AlterDomainStmt);
- n->subtype = 'O';
+ n->subtype = AD_SetNotNull;
n->typeName = $3;
$$ = (Node *) n;
}
@@ -11657,7 +11657,7 @@ AlterDomainStmt:
{
AlterDomainStmt *n =
makeNode(AlterDomainStmt);
- n->subtype = 'C';
+ n->subtype = AD_AddConstraint;
n->typeName = $3;
n->def = $5;
$$ = (Node *) n;
@@ -11667,7 +11667,7 @@ AlterDomainStmt:
{
AlterDomainStmt *n =
makeNode(AlterDomainStmt);
- n->subtype = 'X';
+ n->subtype = AD_DropConstraint;
n->typeName = $3;
n->name = $6;
n->behavior = $7;
@@ -11679,7 +11679,7 @@ AlterDomainStmt:
{
AlterDomainStmt *n =
makeNode(AlterDomainStmt);
- n->subtype = 'X';
+ n->subtype = AD_DropConstraint;
n->typeName = $3;
n->name = $8;
n->behavior = $9;
@@ -11691,7 +11691,7 @@ AlterDomainStmt:
{
AlterDomainStmt *n =
makeNode(AlterDomainStmt);
- n->subtype = 'V';
+
Re: PG 18 release notes draft committed
On Wed, May 28, 2025 at 11:25:03PM +0900, Yugo Nagata wrote: > On Wed, 28 May 2025 23:14:36 +0900 > Yugo Nagata wrote: > > > On Thu, 1 May 2025 22:44:50 -0400 > > Bruce Momjian wrote: > > > > > I have committd the first draft of the PG 18 release notes. The item > > > count looks strong: > > > > Some items in the "EXPLAIN" section are actually not about the EXPLAIN > > but the ANALYZE command. The attached patch is move them to the > > "Utility Commands" section with a little edit of wordings. > > ... or, perhaps they should be moved to the "Monitoring" section. > I've attached a patch in this version. I see you split the ANALYZE and EXPLAIN shared item, and moved the ANALYZE item; patch applied, thanks. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Do not let urgent matters crowd out time for investment in the future.
Re: Document How Commit Handles Aborted Transactions
Recent discussion led me to realize we are also contrary to the SQL Standard here. v3 updates the Commit reference page to reflect this fact. Leaving ready-to-commit. David J. From 9e19152958130f7610feab7983221aace7abfe20 Mon Sep 17 00:00:00 2001 From: "David G. Johnston" Date: Wed, 28 May 2025 16:11:51 -0700 Subject: [PATCH] [PATCH] doc: Commit performs rollback of aborted transactions The Commit command handles an aborted transaction in the same manner as the Rollback command. This needs to be documented. Add it to both the reference page as well mentioning the behavior in the official material for introducting transactions - the tutorial. Note the our behavior is contrary to the SQL Standard as well. In passing, make the description of the Commit reference page self-contained by mentioning the 'and chain' behavior. --- doc/src/sgml/advanced.sgml | 18 +--- doc/src/sgml/ref/commit.sgml | 42 ++-- 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml index e15a3323dfb..db77ca39e73 100644 --- a/doc/src/sgml/advanced.sgml +++ b/doc/src/sgml/advanced.sgml @@ -149,7 +149,8 @@ DETAIL: Key (city)=(Berkeley) is not present in table "cities". systems. The essential point of a transaction is that it bundles multiple steps into a single, all-or-nothing operation. The intermediate states between the steps are not visible to other concurrent transactions, -and if some failure occurs that prevents the transaction from completing, +and if an error +occurs that prevents the transaction from completing, then none of the steps affect the database at all. @@ -218,7 +219,8 @@ UPDATE branches SET balance = balance + 100.00 In PostgreSQL, a transaction is set up by surrounding the SQL commands of the transaction with -BEGIN and COMMIT commands. So our banking + and + commands. So our banking transaction would actually look like: @@ -233,7 +235,7 @@ COMMIT; If, partway through the transaction, we decide we do not want to commit (perhaps we just noticed that Alice's balance went negative), -we can issue the command ROLLBACK instead of +we can issue the command instead of COMMIT, and all our updates so far will be canceled. @@ -256,6 +258,16 @@ COMMIT; + +When an error occurs within a transaction block the transaction is not ended +but instead goes into an aborted state. While in this state all commands except + and are ignored and, +importantly, both those commands behave identically - they roll-back and close +the current transaction, returning the session to a state where new commands can +be issued. They will also automatically begin a new transaction if executed +with an AND CHAIN parameter. + + It's possible to control the statements in a transaction in a more granular fashion through the use of savepoints. Savepoints diff --git a/doc/src/sgml/ref/commit.sgml b/doc/src/sgml/ref/commit.sgml index 7e2dcac5a33..80c2ecd799e 100644 --- a/doc/src/sgml/ref/commit.sgml +++ b/doc/src/sgml/ref/commit.sgml @@ -33,6 +33,19 @@ COMMIT [ WORK | TRANSACTION ] [ AND [ NO ] CHAIN ] changes made by the transaction become visible to others and are guaranteed to be durable if a crash occurs. + + If the transaction is in an aborted state then no changes will be made + and the effect of the COMMIT will be identical to that + of ROLLBACK, including the command tag output. + + + In either situation, if the AND CHAIN parameter is + specified a new, identically configured, transaction is started. + + + For more information regarding transactions see + . + @@ -67,6 +80,25 @@ COMMIT [ WORK | TRANSACTION ] [ AND [ NO ] CHAIN ] + + Outputs + + + On successful completion on a non-aborted transaction, a COMMIT + command returns a command tag of the form + +COMMIT + + + + However, on an aborted transaction, a COMMIT + command returns a command tag of the form + +ROLLBACK + + + + Notes @@ -96,8 +128,13 @@ COMMIT; Compatibility - The command COMMIT conforms to the SQL standard. The - form COMMIT TRANSACTION is a PostgreSQL extension. + The command COMMIT conforms to the SQL standard, except + that no exception condition is raised in the case where an already aborted + transaction is committed. + + + + The form COMMIT TRANSACTION is a PostgreSQL extension. @@ -107,6 +144,7 @@ COMMIT; + -- 2.34.1
Re: pg16 && GSSAPI && Heimdal/Macos
On Wed, May 28, 2025 at 9:25 AM Jacob Champion
wrote:
> Personally, I'd be more happy to "maintain GSS on Mac using
> non-deprecated interfaces" than "maintain GSS via Heimdal,
> best-effort, some of the time". I think the former puts less of a
> burden on our testing matrix.
I was curious enough to put in some time to get GSS.framework
compiling via Autoconf, and I might as well share the ugly code I've
got. There are some similarities to Todd's earlier patch, but
decisions are made at different places; it detects either MIT Kerberos
or GSS.framework. And I haven't looked at the Meson side yet.
- I am not well-versed in frameworks. There's a bunch of namespace
pollution in Apple's GSS headers, and I'm hoping I'm missing some
magic #define to make that all go away.
- My handling of pg_store_delegated_credential() here isn't something
I'm seriously proposing. I think we should find a way to get it
working on Mac, using Nico's notes upthread. I can't commit to working
on that myself, but I'm definitely willing to put some review cycles
in, since I reviewed a bit of the original delegation feature.
- I also want to draw attention to the fact that libpq can't claim
that a credential is delegated if it's not; that breaks the security
of our FDWs. So pg_store_delegated_credential() cannot be a no-op.
--Jacob
From d708e3ecd157fe7e990725d8377b5613c0ce5bdb Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Wed, 28 May 2025 10:18:50 -0700
Subject: [PATCH 1/2] WIP: move GSSAPI checks into their own macro
---
config/programs.m4 | 15 +++
configure | 2 ++
configure.ac | 7 +--
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/config/programs.m4 b/config/programs.m4
index 0ad1e58b48d..f2a950750bd 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -354,3 +354,18 @@ AC_DEFUN([PGAC_CHECK_LIBCURL],
LDFLAGS=$pgac_save_LDFLAGS
LIBS=$pgac_save_LIBS
])# PGAC_CHECK_LIBCURL
+
+
+# PGAC_CHECK_GSSAPI
+# --
+# Check for a GSSAPI implementation.
+
+AC_DEFUN([PGAC_CHECK_GSSAPI],
+[
+ if test "$PORTNAME" != "win32"; then
+AC_SEARCH_LIBS(gss_store_cred_into, [gssapi_krb5 gss 'gssapi -lkrb5 -lcrypto'], [],
+ [AC_MSG_ERROR([could not find function 'gss_store_cred_into' required for GSSAPI])])
+ else
+LIBS="$LIBS -lgssapi32"
+ fi
+])# PGAC_CHECK_GSSAPI
diff --git a/configure b/configure
index 4f15347cc95..327b2de8184 100755
--- a/configure
+++ b/configure
@@ -12892,6 +12892,7 @@ $as_echo "$pgac_cv__libcurl_async_dns" >&6; }
fi
if test "$with_gssapi" = yes ; then
+
if test "$PORTNAME" != "win32"; then
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing gss_store_cred_into" >&5
$as_echo_n "checking for library containing gss_store_cred_into... " >&6; }
@@ -12954,6 +12955,7 @@ fi
else
LIBS="$LIBS -lgssapi32"
fi
+
fi
#
diff --git a/configure.ac b/configure.ac
index 4b8335dc613..97d9375703f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1367,12 +1367,7 @@ if test "$with_libcurl" = yes ; then
fi
if test "$with_gssapi" = yes ; then
- if test "$PORTNAME" != "win32"; then
-AC_SEARCH_LIBS(gss_store_cred_into, [gssapi_krb5 gss 'gssapi -lkrb5 -lcrypto'], [],
- [AC_MSG_ERROR([could not find function 'gss_store_cred_into' required for GSSAPI])])
- else
-LIBS="$LIBS -lgssapi32"
- fi
+ PGAC_CHECK_GSSAPI
fi
#
--
2.34.1
From efb9dda2254db3cbcb7e915b652ecbb20c6ef2be Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Wed, 28 May 2025 11:09:15 -0700
Subject: [PATCH 2/2] WIP: fall back to GSS.framework on macOS
---
config/programs.m4 | 27 ++-
configure| 106 ++-
configure.ac | 12 ++-
src/backend/libpq/auth.c | 6 ++
src/backend/libpq/be-gssapi-common.c | 4 +
src/backend/libpq/be-secure-gssapi.c | 6 ++
src/include/libpq/be-gssapi-common.h | 3 +
src/include/libpq/pg-gssapi.h| 13 +++-
src/include/pg_config.h.in | 6 ++
9 files changed, 172 insertions(+), 11 deletions(-)
diff --git a/config/programs.m4 b/config/programs.m4
index f2a950750bd..5a218bd9e63 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -358,14 +358,35 @@ AC_DEFUN([PGAC_CHECK_LIBCURL],
# PGAC_CHECK_GSSAPI
# --
-# Check for a GSSAPI implementation.
+# Check for a GSSAPI implementation. pgac_found_gss is set to 'yes' if we find
+# MIT Kerberos, or 'framework' if we find GSS.framework on Mac.
AC_DEFUN([PGAC_CHECK_GSSAPI],
[
if test "$PORTNAME" != "win32"; then
-AC_SEARCH_LIBS(gss_store_cred_into, [gssapi_krb5 gss 'gssapi -lkrb5 -lcrypto'], [],
- [AC_MSG_ERROR([could not find function 'gss_store_cred_into' required for GSSAPI])])
+pgac_found_gss=no
+
+AC_SEARCH_LIBS(gss_store_cred_into, [gssapi_krb5 gss 'gssapi -lkrb5 -lcrypto'],
+ [pgac_found_gss=yes])
+
+# If
Re: Correct documentation for protocol version
On Tue, 22 Apr 2025 at 10:34, Dave Cramer wrote: > > On Fri, 11 Apr 2025 at 11:02, Jelte Fennema-Nio > wrote: > >> On Fri, 11 Apr 2025 at 22:57, Dave Cramer wrote: >> > Well this isn't quite true since if you request 3.0 and have invalid >> options it will return 3.0, which is not the highest supported minor >> version. >> >> Probably good to update this section too then to be similarly correct >> as your already updated section. Maybe also good to clarify further >> that the version that the server responds with is the protocol version >> that will be used during the following communication. >> > > I've updated the wording to specify that the negotiateProtocol message > will only be sent if the client requests a major version the server > supports. > Also added a note saying that this will be the protocol version that will > be used for the duration of the connectin > > I found another place where the docs should be updated. The Changes since Protocol 2.0 See attached patch > > protocol-7.patch Description: Binary data
Re: PG 18 release notes draft committed
On Wed, May 28, 2025 at 02:54:29PM -0400, Joe Conway wrote: > On 5/27/25 17:27, Bruce Momjian wrote: > > On Mon, May 26, 2025 at 10:20:08AM -0400, Joe Conway wrote: > > > On 5/23/25 09:47, Bruce Momjian wrote: > > > > On Fri, May 23, 2025 at 09:54:54AM +0200, Álvaro Herrera wrote: > > > > > I also think that showing an XML-ish format of a commit message is > > > > > unhelpful, not to mention hard to read. Showing a few actual examples > > > > > would be better. > > > > > Agreed, but the XML came from Joe Conway so I am hesitant to > > > remove it > > > > myself without feedback from him. > > > > > > > > > I am not married to it, but I would say that I find pure examples > > > confusing/ambiguous. To me at least the XML-ish specification is easier to > > > understand. > > > > > > Perhaps both the specification as-is and one or two examples added? > > > > Yeah, I think some people like syntax, others like examples. > > What do you think about providing links into the archives for good > representative log messages rather than making up a contrived example and/or > copy/pasting them into the wiki page itself? Sure, we can just link to: https://www.postgresql.org/list/pgsql-committers/ and they can choose any recent commit. > Also, looking at the wiki page, my inclination would be to add an "Examples" > section at the bottom of the wiki page -- does that work or do you think it > ought to go just under the "General layout" syntax section? Sure, makes sense. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Do not let urgent matters crowd out time for investment in the future.
Re: PG 18 release notes draft committed
On Thu, May 1, 2025 at 10:44 PM Bruce Momjian wrote: > > I will continue improving it until beta 1, and until the final release. Hi Bruce, Thanks so much for putting these together. For the item: "Increase the logging granularity of server variable log_connections" I noticed that you cite the commit 9219093cab2 that actually does modularize the GUC but you also cite a separate following commit 18cd15e706ac which adds a new option that logs the duration of various parts of connection establishment and backend setup. That is, it is a separate feature. 9219093cab2 made it so we could add options and have them be individually enabled or disabled in logging. But 18cd15e706ac is only related insomuch as we probably wouldn't have added it if log_connections had been just a boolean and it was enabled by default. Anyway, it might be worth separately calling out that now you can configure log_connections to log the durations of various parts of connection establishment and backend setup -- which is a distinct feature from modularization. For the item: "Add an asynchronous I/O subsystem" I notice we don't call out any of the operations where users could expect to see asynchronous IO be used. Some were enabled in 17 (like sequential scans, analyze, and pg_prewarm), but most of the read stream users went in this release: d9c7911e1a5, 043799fa08c, e215166c9c8, 69273b818b1, c5c239e26e3, 2b73a8cd33b, 9256822608f, c3e775e608f, 8720a15e9ab12, 65c310b310a I have had users ask me already which operations they can expect to use asynchronous I/O. The most commonly encountered AIO operations are probably be vacuum, bitmap heap scan, and sequential scans, but it might be worth having a list somewhere of what uses AIO. I expect we'll get the question quite often. And finally, for the item: "Allow specification of the fixed number of dead tuples that will trigger an autovacuum" We also added a kind of corollary for insert-triggered vacuums in 06eae9e6218ab2a which attempts to deal with a similar problem of big tables not being autovacuumed enough but for insert-mostly tables. Perhaps because there is no exposed configuration it is not worth mentioning, but I thought I would bring it up since their purposes are related. - Melanie
Re: Conflict detection for update_deleted in logical replication
On Tue, May 27, 2025 at 11:45 AM Amit Kapila wrote: > > On Mon, May 26, 2025 at 12:46 PM Zhijie Hou (Fujitsu) > wrote: > > > > On Sun, May 25, 2025 at 4:36 PM Dilip Kumar wrote: > > > > > > > > I am thinking can't we make it more deterministic such that when we > > > get the status first time if we find some transactions that are in > > > commit phase then we should just wait for those transaction to get > > > committed? One idea is to get the list of xids in commit phase and > > > next time when we get the list we can just compare and in next status > > > if we don't get any xids in commit phase which were in commit phase > > > during previous status then we are done. But not sure is this worth > > > the complexity? Mabe not but shall we add some comment explaining the > > > case and also explaining why this corner case is not harmful? > > > > I also think it's not worth the complexity for this corner case which is > > rare. > > Yeah, complexity is one part, but I feel improving such less often > cases could add performance burden for more often cases where we need > to either maintain and invalidate the cache on the publisher or send > the list of all such xids to the subscriber over the network. Yeah, that's a valid point. > > So, I have added some comments in wait_for_publisher_status() to > > mention the same. > > > > I agree that at this stage it is good to note down in comments, and if > we face such cases often, then we can improve it in the future. +1 -- Regards, Dilip Kumar Google
Re: queryId constant squashing does not support prepared statements
On Wed, May 28, 2025 at 04:05:03PM -0500, Sami Imseih wrote: > That's my mistake. I added a new file called normalize.sql to test > specific normalization scenarios. Added in v7 Thanks. I was not sure that a new file was worth having for these tests, knowing that select.sql has similar coverage. I have grouped the new tests into select.sql at the end, and added a few more scenarios for extended queries with \bindin extended.sql, where I've reproduced the same issue while playing with external parameters. Applied the result down to v13, down to where the problem exists for supported branches. I still need to review the rest of the patch series.. -- Michael signature.asc Description: PGP signature
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Thanks for your feedback. I've attached the updated patches.
On 2025-05-28 10:10, Fujii Masao wrote:
On 2025/05/26 16:55, ikedamsh wrote:
2025/05/21 12:54 Fujii Masao :
Regarding the 0002 patch:
- errdetail("Relation \"%s\" is a %s index.",
-RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname;
+ errdetail("Relation \"%s\" is a %s %sindex.",
+RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname),
+(rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) ?
"partitioned " : "")));
Instead of manually building the message, how about using
errdetail_relkind_not_supported()?
It would be more consistent with similar error reporting
elsewhere.
I was thinking of using errdetail_relkind_not_supported(),
but I’m reconsidering building the message manually
since the AM name seems to be important for the error.
What do you think?
Understood.
I was trying to better understand the error message, as I found
the following is still a bit confusing for users. However, I don't
have a better suggestion at the moment, so I'm okay with
the proposed change.
ERROR: expected "btree" index as targets for verification
DETAIL: Relation "pgbench_accounts_pkey" is a btree partitioned
What do you think about adding new error messages specifically for
partitioned
indexes?
If the target is a partitioned index, the error messages would be:
ERROR: expected index as targets for verification
DETAIL: This operation is not supported for partitioned indexes.
If the target is an index, but its access method is not supported, the
error
messages would be:
ERROR: expected "btree" index as targets for verification
DETAIL: Relation "bttest_a_brin_idx" is a brin index.
This is not directly relation to your proposal, but while reading
the index_checkable() function, I noticed that ReleaseSysCache()
is not called after SearchSysCache1(). Shouldn't we call
ReleaseSysCache() here? Alternatively, we could use get_am_name()
instead of SearchSysCache1(), which might be simpler.
Agreed.
I also observed that the error code ERRCODE_FEATURE_NOT_SUPPORTED
is used when the relation is not the expected type in
index_checkable().
However, based on similar cases (e.g., pgstattuple), it seems that
ERRCODE_WRONG_OBJECT_TYPE might be more appropriate in this situation.
Thought?
Agreed. I also change the error code to
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE
when the index is not valid.
Regards,
--
Masahiro Ikeda
NTT DATA Japan CorporationFrom c1c7c06a1d2fd7f39a22a81679bfbefb5e0b1911 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
Date: Mon, 26 May 2025 16:25:42 +0900
Subject: [PATCH v4 1/3] Fix assertion failure when pg_prewarm() is run on
objects that don't have storage.
This issue was introduced by commit 049ef33.
Specifying objects that don't have storage as an argument to
pg_prewarm() could trigger an assertion failure:
Failed Assert("RelFileNumberIsValid(rlocator.relNumber)")
This fix ensures that such cases are handled appropriately.
---
contrib/pg_prewarm/Makefile| 2 ++
contrib/pg_prewarm/expected/pg_prewarm.out | 10 ++
contrib/pg_prewarm/meson.build | 5 +
contrib/pg_prewarm/pg_prewarm.c| 8
contrib/pg_prewarm/sql/pg_prewarm.sql | 10 ++
5 files changed, 35 insertions(+)
create mode 100644 contrib/pg_prewarm/expected/pg_prewarm.out
create mode 100644 contrib/pg_prewarm/sql/pg_prewarm.sql
diff --git a/contrib/pg_prewarm/Makefile b/contrib/pg_prewarm/Makefile
index 9cfde8c4e4f..617ac8e09b2 100644
--- a/contrib/pg_prewarm/Makefile
+++ b/contrib/pg_prewarm/Makefile
@@ -10,6 +10,8 @@ EXTENSION = pg_prewarm
DATA = pg_prewarm--1.1--1.2.sql pg_prewarm--1.1.sql pg_prewarm--1.0--1.1.sql
PGFILEDESC = "pg_prewarm - preload relation data into system buffer cache"
+REGRESS = pg_prewarm
+
TAP_TESTS = 1
ifdef USE_PGXS
diff --git a/contrib/pg_prewarm/expected/pg_prewarm.out b/contrib/pg_prewarm/expected/pg_prewarm.out
new file mode 100644
index 000..94e4fa1a9d2
--- /dev/null
+++ b/contrib/pg_prewarm/expected/pg_prewarm.out
@@ -0,0 +1,10 @@
+-- Test pg_prewarm extension
+CREATE EXTENSION pg_prewarm;
+-- pg_prewarm() should fail if the target relation has no storage.
+CREATE TABLE test (c1 int) PARTITION BY RANGE (c1);
+SELECT pg_prewarm('test', 'buffer');
+ERROR: relation "test" does not have storage
+DETAIL: This operation is not supported for partitioned tables.
+-- Cleanup
+DROP TABLE test;
+DROP EXTENSION pg_prewarm;
diff --git a/contrib/pg_prewarm/meson.build b/contrib/pg_prewarm/meson.build
index 82b9851303c..f24c47ef6a5 100644
--- a/contrib/pg_prewarm/meson.build
+++ b/contrib/pg_prewarm/meson.build
@@ -29,6 +29,11 @@ tests += {
'name': 'pg_prewarm',
'sd': meson.current_source_dir(),
'bd': meson.current_build_dir(),
+ 'regress': {
+'sql': [
+ 'pg_prewarm',
+],
+ },
'tap': {
't
Re: PG 18 release notes draft committed
On Wed, May 28, 2025 at 08:07:20PM -0400, Melanie Plageman wrote: > For the item: > > "Increase the logging granularity of server variable log_connections" > > I noticed that you cite the commit 9219093cab2 that actually does > modularize the GUC but you also cite a separate following commit > 18cd15e706ac which adds a new option that logs the duration of various > parts of connection establishment and backend setup. That is, it is a > separate feature. > > 9219093cab2 made it so we could add options and have them be > individually enabled or disabled in logging. But 18cd15e706ac is only > related insomuch as we probably wouldn't have added it if > log_connections had been just a boolean and it was enabled by default. > > Anyway, it might be worth separately calling out that now you can > configure log_connections to log the durations of various parts of > connection establishment and backend setup -- which is a distinct > feature from modularization. Yes, I can now see it is two items so I have split it into two in the attached, applied patch. In a separate commit I adjusted the docs for log_connections to more clearly explain the new "setup_durations" output. > For the item: > > "Add an asynchronous I/O subsystem" > > I notice we don't call out any of the operations where users could > expect to see asynchronous IO be used. Some were enabled in 17 (like > sequential scans, analyze, and pg_prewarm), but most of the read > stream users went in this release: > > d9c7911e1a5, 043799fa08c, e215166c9c8, 69273b818b1, c5c239e26e3, > 2b73a8cd33b, 9256822608f, c3e775e608f, 8720a15e9ab12, 65c310b310a > > I have had users ask me already which operations they can expect to > use asynchronous I/O. The most commonly encountered AIO operations are > probably be vacuum, bitmap heap scan, and sequential scans, but it > might be worth having a list somewhere of what uses AIO. I expect > we'll get the question quite often. Yes, I knew I needed more detail on this. I have added text in this commit to try to improve that. > And finally, for the item: > > "Allow specification of the fixed number of dead tuples that will > trigger an autovacuum" > > We also added a kind of corollary for insert-triggered vacuums in > 06eae9e6218ab2a which attempts to deal with a similar problem of big > tables not being autovacuumed enough but for insert-mostly tables. > Perhaps because there is no exposed configuration it is not worth > mentioning, but I thought I would bring it up since their purposes are > related. I studied this and I can't figure out how to clearly explain it in a useful way. I am now thinking it is more of a bug or behavior fix or that would not be usually mentioned. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Do not let urgent matters crowd out time for investment in the future. commit a1de1b0833b Author: Bruce Momjian Date: Wed May 28 22:43:13 2025 -0400 doc PG 18 relnotes: split apart log_connections item Also add details to asynchronous I/O item. Reported-by: Melanie Plageman Discussion: https://postgr.es/m/CAAKRu_YsVvyantS0X0Y_-vp_97=ygaoyjmxxycekr7pumah...@mail.gmail.com diff --git a/doc/src/sgml/release-18.sgml b/doc/src/sgml/release-18.sgml index 83ff2225396..718f7b20bf1 100644 --- a/doc/src/sgml/release-18.sgml +++ b/doc/src/sgml/release-18.sgml @@ -569,6 +569,7 @@ Add an asynchronous I/O subsystem (Andres Freund, Thomas Munro, Nazir Bilal Yavu +This feature allows backends to queue multiple read requests, which allows for more efficient sequential scans, bitmap heap scans, and vacuums. This is enabled by server variable io_method, with server variables io_combine_limit and io_max_combine_limit added to control it. This also enables effective_io_concurrency and maintenance_io_concurrency values greater than zero for systems without fadvise() support. The new system view pg_aios shows the file handles being used for asynchronous I/O. @@ -682,15 +683,12 @@ This more accurately reflects modern hardware. Increase the logging granularity of server variable log_connections (Melanie Plageman) § -§ @@ -698,6 +696,18 @@ This server variable was previously only boolean; these options are still suppo + + + + +Add log_connections option to report the duration of connection stages (Melanie Plageman) +§ + + +
Re: Add comment explaining why queryid is int64 in pg_stat_statements
On Thu, May 22, 2025 at 01:01:14PM +0900, Michael Paquier wrote: > I have added an open item about the plan ID part as it applies to v18, > adding the RMT in CC to get an opinion. If we cannot get a consensus > on all that, letting things as they are is still logically correct, > even with the -Wwarnings-format-signedness argument which is not > included by default currently. > > Has somebody an opinion to offer? It has been one week since this last update, and there has been nothing except the sound of cicadas. IMO, I think that we should just pull the switch and make both of these IDs signed on HEAD, taking case of the potential signedness warning issues. Now, I don't really want to take a leap of faith without the RMT being OK with that now that we are in beta1. -- Michael signature.asc Description: PGP signature
Re: Virtual generated columns
On Fri, May 16, 2025 at 5:35 PM jian he wrote: > we have used the USING expression in ATPrepAlterColumnType, > ATColumnChangeRequiresRewrite. > expanding it on ATPrepAlterColumnType seems to make more sense? > > @@ -14467,7 +14467,7 @@ ATPrepAlterColumnType(List **wqueue, > */ > newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue)); > newval->attnum = attnum; > - newval->expr = (Expr *) transform; > + newval->expr = (Expr *) > expand_generated_columns_in_expr(transform, rel, 1); > newval->is_generated = false; Yeah, ATPrepAlterColumnType does seem like a better place. But we need to ensure that ATColumnChangeRequiresRewrite sees the expanded version of the expression — your proposed change fails to do that. Additionally, I think we also need to ensure that the virtual generated columns are expanded before the expression is fed through expression_planner, to ensure it can be successfully transformed into an executable form. Hence, the attached patch. Thanks Richard v1-0001-Expand-virtual-generated-columns-for-ALTER-COLUMN.patch Description: Binary data
Re: Foreign key validation failure in 18beta1
Tender Wang 于2025年5月28日周三 20:38写道： > > > Alvaro Herrera 于2025年5月28日周三 20:26写道： > >> On 2025-May-28, Tender Wang wrote: >> >> > I dided the codes, in QueueFKConstraintValidation(), we add three >> > newconstraint for the >> > fk rel, because the pk rel is partition table. >> > >> > During phase 3 of AlterTable, in ATRewriteTables(), >> > call validateForeignKeyConstraint() three times. >> > The first time the pk rel is pk, and it's ok. >> > The second time the pk rel is only pk_1, and the type(1) is not in >> pk_1, so >> > an error is reported. >> > >> > In this case, the two children newconstraint should not be added to the >> > queue. >> >> Yeah, I reached the same conclusion and this is the preliminary fix I >> had written for it. I don't like that I had to duplicate a few lines of >> code, but maybe it's not too bad. Also the comments need to be >> clarified a bit more. >> > > If the child table is still a partitioned table, the patch seems not work. > I found a case that proves what I said above. create table pk(i int, j int, primary key(i,j)) partition by range (i); create table pk_1 partition of pk for values from (0) to (1) partition by list(j); create table pk_2 partition of pk for values from (1) to (2) partition by list(j); create table pk_1_1 partition of pk_1 for values in (1); create table pk_2_1 partition of pk_2 for values in (2); create table fk(i int, j int); alter table fk add foreign key(i, j) references pk not valid; postgres=# select oid ,conname , contype, convalidated,conrelid,conparentid,confrelid from pg_constraint where oid >= 16384; oid |conname| contype | convalidated | conrelid | conparentid | confrelid ---+---+-+--+--+-+--- 16422 | fk_i_j_fkey | f | f|16419 | 0 | 16384 16425 | fk_i_j_fkey_1 | f | f|16419 | 16422 | 16391 16428 | fk_i_j_fkey_2 | f | f|16419 | 16425 | 16405 16431 | fk_i_j_fkey_3 | f | f|16419 | 16422 | 16398 16434 | fk_i_j_fkey_4 | f | f|16419 | 16431 | 16412 alter table fk validate constraint fk_i_j_fkey; postgres=# select oid ,conname , contype, convalidated,conrelid,conparentid,confrelid from pg_constraint where oid >= 16384; oid |conname| contype | convalidated | conrelid | conparentid | confrelid ---+---+-+--+--+-+--- 16428 | fk_i_j_fkey_2 | f | f|16419 | 16425 | 16405 16434 | fk_i_j_fkey_4 | f | f|16419 | 16431 | 16412 16425 | fk_i_j_fkey_1 | f | t|16419 | 16422 | 16391 16431 | fk_i_j_fkey_3 | f | t|16419 | 16422 | 16398 16422 | fk_i_j_fkey | f | t|16419 | 0 | 16384 The fk_i_j_fkey_2 and fk_i_j_fkey_4 are still invalid with your patch. > I figure out a quick fix as the attached. I add a bool argument into > the QueueFKConstraintValidation(). > If it is true, it means we recursively call QueueFKConstraintValidation(), > then we don't add the newconstraint to the queue. > > I'm not sure about this fix. Any thoughts? > -- Thanks, Tender Wang
Re: Proposals for EXPLAIN: rename ANALYZE to EXECUTE and extend VERBOSE
On 2024/12/14 0:59, Michael Christofides wrote: > I've pushed the main patch. Woohoo! And thank you. I've already seen quite a lot of positivity around the commit on Twitter[1][2][3]. > I'm not planning on pushing the auto_explain.log_buffers default change unless there's a bit more discussion about it. Much like Guillaume, I'd also be in favour of 0002, but it's nowhere near as important to me. I think most people consider the parameters far more when setting up auto_explain, so I believe far fewer omit buffers by mistake. Also, most cloud providers don't ship with auto_explain on, and the only one I know of that does[4], ships with log_buffers on too. On the plus side, it would be nice to be consistent. But on the downside, it might add a little extra overhead for folks who run auto_explain with log_analyze on, and who opted not to set log_buffers and upgrade without setting it to off explicitly. I am still in favour of the 0002 patch being applied, to avoid confusion and maximise the chance people that don't know about buffers still get them in their plans. I agree with changing the default value of auto_explain.log_buffers to true. I think that users who know EXPLAIN ANALYZE includes buffers info in the result by default since v18 would expect the buffer info also to be included in auto_explain output as long as log_analyze is enabled. So setting log_buffers to true by default would be less confusing. As for 0002 patch, should the example log output with log_analyze enabled also be updated to show the buffer information? Sorry for reviving this old thread, but while testing the new v18 feature - buffers being shown by default - I had the same thought about auto_explain.log_buffers, so I wanted to chime in. Regards, -- Fujii Masao NTT DATA Japan Corporation
Re: Add AioUringCompletion in wait_event_names.txt and a safeguard in generate-wait_event_types.pl
On Tue, May 27, 2025 at 08:30:56AM +0900, Michael Paquier wrote: > +AioUringCompletion "Waiting for another process to complete IO via io_uring." > > "completion_lock" is described with similar words around the top of > method_io_uring.c. In more exact words, we are waiting for a backend > to process IO completions with io_method=io_uring. So perhaps: > "Waiting for another process to process IO completions with io_uring" Hearing nothing, I've applied this one reusing the exact wording suggested by Bertrand which is the most consistent choice with the other wait events related to the same area of the code. -- Michael signature.asc Description: PGP signature
backend_xmin is null in pg_stat_replication, although repl. slot change it
Hi why, backend_xmin from pg_stat_replication is not updated, although used related replication slot effectively use something like this and blocks removing dead tuples? postgres=# select slot_name, age(xmin) from pg_replication_slots; -[ RECORD 1 ]- slot_name | readonly01 age | 11162 -[ RECORD 2 ]- slot_name | readonly02 age | 196 postgres=# select * from pg_stat_replication; -[ RECORD 1 ]+-- pid | 7715 usesysid | 16597 usename | repmgr application_name | walreceiver client_addr | x.2 client_hostname | client_port | 56266 backend_start| 2025-02-20 01:05:50.015719+01 backend_xmin | state| streaming sent_lsn | E154/773EA620 write_lsn| E154/773EA620 flush_lsn| E154/773EA620 replay_lsn | E154/773EA620 write_lag| 00:00:00.00024 flush_lag| 00:00:00.000625 replay_lag | 00:00:00.000754 sync_priority| 0 sync_state | async reply_time | 2025-05-29 07:03:48.827431+02 -[ RECORD 2 ]+-- pid | 4097782 usesysid | 16597 usename | repmgr application_name | walreceiver client_addr | x.1 client_hostname | client_port | 42806 backend_start| 2025-05-28 05:12:43.210414+02 backend_xmin | state| streaming sent_lsn | E154/4F31E000 write_lsn| E154/4F00 flush_lsn| E154/4F00 replay_lsn | E154/2141B1C8 write_lag| 00:02:06.153229 flush_lag| 00:02:06.153229 replay_lag | 00:02:48.65516 sync_priority| 0 sync_state | async reply_time | 2025-05-29 07:03:48.818324+02 It is a little bit confusing. Regards Pavel
Re: Clarification on warning when connecting to 'pgbouncer' database via Pgbouncer
> The entire point of having a wire protocol is that you can't/don't > need to detect that. The short answer here is that this is > pgbouncer's bug and you should be nagging them to change, not us. Assuming we've requested the PgBouncer team to stop abusing the server_version parameter for the special 'pgbouncer' database, and they’ve agreed to pass the actual PostgreSQL server version instead - this would prevent us from showing the usual warning, since the version would now appear to be > 9.2 But, still most of the psql features won't work here, right? Won't it be a false positive to not show the warning here? Please correct me if I'm wrong. Thanks & Regards, Shaik Mohammad Mujeeb Member Technical Staff https://www.zoho.com/ On Wed, 28 May 2025 04:51:05 +0530 Tom Lane wrote --- Shaik Mohammad Mujeeb < mailto:mujeeb...@zohocorp.com > writes: > So, I feel that if we can reliably detect when the backend is a > non-PostgreSQL server, it might be better to adjust the warning accordingly, > rather than relying on a client-server version comparison in such cases. The entire point of having a wire protocol is that you can't/don't need to detect that. The short answer here is that this is pgbouncer's bug and you should be nagging them to change, not us. regards, tom lane
Re: Logical Replication of sequences
On Fri, May 23, 2025 at 3:12 AM vignesh C wrote:
>
>
> The attached v20250522 patch has the changes for the same.
>
> Regards,
> Vignesh
>
Some review comments for patch 0001:
1. In src/backend/commands/sequence.c
in pg_sequence_state()
+ /* open and lock sequence */
+ init_sequence(seq_relid, &elm, &seqrel);
+
+ if (pg_class_aclcheck(elm->relid, GetUserId(),
+ ACL_SELECT | ACL_USAGE) != ACLCHECK_OK)
+ ereport(ERROR,
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied for sequence %s",
+ RelationGetRelationName(seqrel)));
+
How about using aclcheck_error for this, which also supports error messages
for specific access errors. Most other objects seem to be using this.
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(seqrel->relkind),
RelationGetRelationName(seqrel));
2. in function pg_sequence_state()
+
+ UnlockReleaseBuffer(buf);
+ relation_close(seqrel, NoLock);
Ideally the corresponding close for init_sequence is sequence_close()
rather than relation_close()
I will post comments for the other patches as well.
regards,
Ajin Cherian
Fujitsu Australia
