public inbox for [email protected]help / color / mirror / Atom feed
Re: Disallow quorum uncommitted (with synchronous standbys) txns in logical replication subscribers 5+ messages / 4 participants [nested] [flat]
* Re: Disallow quorum uncommitted (with synchronous standbys) txns in logical replication subscribers @ 2022-01-07 17:44 SATYANARAYANA NARLAPURAM <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: SATYANARAYANA NARLAPURAM @ 2022-01-07 17:44 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: Jeff Davis <[email protected]>; Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Jan 7, 2022 at 12:27 AM Kyotaro Horiguchi <[email protected]> wrote: > At Thu, 6 Jan 2022 23:55:01 -0800, SATYANARAYANA NARLAPURAM < > [email protected]> wrote in > > On Thu, Jan 6, 2022 at 11:24 PM Jeff Davis <[email protected]> wrote: > > > > > On Wed, 2022-01-05 at 23:59 -0800, SATYANARAYANA NARLAPURAM wrote: > > > > I would like to propose a GUC send_Wal_after_quorum_committed which > > > > when set to ON, walsenders corresponds to async standbys and logical > > > > replication workers wait until the LSN is quorum committed on the > > > > primary before sending it to the standby. This not only simplifies > > > > the post failover steps but avoids unnecessary downtime for the async > > > > replicas. Thoughts? > > > > > > Do we need a GUC? Or should we just always require that sync rep is > > > satisfied before sending to async replicas? > > > > > > > I proposed a GUC to not introduce a behavior change by default. I have no > > strong opinion on having a GUC or making the proposed behavior default, > > would love to get others' perspectives as well. > > > > > > > > > > It feels like the sync quorum should always be ahead of the async > > > replicas. Unless I'm missing a use case, or there is some kind of > > > performance gotcha. > > > > > > > I couldn't think of a case that can cause serious performance issues but > > will run some experiments on this and post the numbers. > > I think Jeff is saying that "quorum commit" already by definition > means that all out-of-quorum standbys are behind of the > quorum-standbys. I agree to that in a dictionary sense. But I can > think of the case where the response from the top-runner standby > vanishes or gets caught somewhere on network for some reason. In that > case the primary happily checks quorum ignoring the top-runner. > > To avoid that misdecision, I can guess two possible "solutions". > > One is to serialize WAL sending (of course it is unacceptable at all) > or aotehr is to send WAL to all standbys at once then make the > decision after making sure receiving replies from all standbys (this > is no longer quorum commit in another sense..) > There is no need to serialize sending the WAL among sync standbys. The only serialization required is first to all the sync replicas and then to sync replicas if any. Once an LSN is quorum committed, no failover subsystem initiates an automatic failover such that the LSN is lost (data loss) > > So I'm afraid that there's no sensible solution to avoid the > hiding-forerunner problem on quorum commit. > Could you elaborate on the problem here? > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Disallow quorum uncommitted (with synchronous standbys) txns in logical replication subscribers @ 2022-01-07 18:23 Bossart, Nathan <[email protected]> parent: SATYANARAYANA NARLAPURAM <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Bossart, Nathan @ 2022-01-07 18:23 UTC (permalink / raw) To: SATYANARAYANA NARLAPURAM <[email protected]>; Kyotaro Horiguchi <[email protected]>; +Cc: Jeff Davis <[email protected]>; Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> On 1/6/22, 11:25 PM, "Jeff Davis" <[email protected]> wrote: > On Wed, 2022-01-05 at 23:59 -0800, SATYANARAYANA NARLAPURAM wrote: >> I would like to propose a GUC send_Wal_after_quorum_committed which >> when set to ON, walsenders corresponds to async standbys and logical >> replication workers wait until the LSN is quorum committed on the >> primary before sending it to the standby. This not only simplifies >> the post failover steps but avoids unnecessary downtime for the async >> replicas. Thoughts? > > Do we need a GUC? Or should we just always require that sync rep is > satisfied before sending to async replicas? > > It feels like the sync quorum should always be ahead of the async > replicas. Unless I'm missing a use case, or there is some kind of > performance gotcha. I don't have a strong opinion on whether there needs to be a GUC, but +1 for the ability to enforce sync quorum before sending WAL to async standbys. I think that would be a reasonable default behavior. Nathan ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Disallow quorum uncommitted (with synchronous standbys) txns in logical replication subscribers @ 2022-01-11 08:30 Kyotaro Horiguchi <[email protected]> parent: SATYANARAYANA NARLAPURAM <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2022-01-11 08:30 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected] At Fri, 7 Jan 2022 09:44:15 -0800, SATYANARAYANA NARLAPURAM <[email protected]> wrote in > On Fri, Jan 7, 2022 at 12:27 AM Kyotaro Horiguchi <[email protected]> > wrote: > > One is to serialize WAL sending (of course it is unacceptable at all) > > or aotehr is to send WAL to all standbys at once then make the > > decision after making sure receiving replies from all standbys (this > > is no longer quorum commit in another sense..) > > > > There is no need to serialize sending the WAL among sync standbys. The only > serialization required is first to all the sync replicas and then to sync > replicas if any. Once an LSN is quorum committed, no failover subsystem > initiates an automatic failover such that the LSN is lost (data loss) Sync standbys on PostgreSQL is ex post facto. When a certain set of standbys have first reported catching-up for a commit, they are called "sync standbys". We can maintain a fixed set of sync standbys based on the set of sync-standbys at a past commits, but that implies performance degradation even if not a single standby is gone. If we send WAL only to the fixed-set of sync standbys, when any of the standbys is gone, the primary is forced to wait until some timeout expires. The same commit would finish immediately if WAL had been sent also to out-of-quorum standbys. > > So I'm afraid that there's no sensible solution to avoid the > > hiding-forerunner problem on quorum commit. > > Could you elaborate on the problem here? If a primary have received response for LSN=X from N standbys, that fact doesn't guarantee that none of the other standbys reached the same LSN. If one of the yet-unresponded standbys already reached LSN=X+10 but its response does not arrived to the primary for some reasons, the true-fastest standby is hiding from primary. Even if the primary examines the responses from all standbys, it is uncertain if the responses reflect the truly current state of the standbys. Thus if we want to guarantee that no unresponded standby is going beyond LSN=X, there's no means other than we refrain from sending WAL beyond X. In that case, we need to serialize the period from WAL-sending to response-reception, which would lead to critical performance degradation. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH] CREATE INDEX: use the original userid for more ACL checks. @ 2022-06-25 16:07 Noah Misch <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Noah Misch @ 2022-06-25 16:07 UTC (permalink / raw) Commit a117cebd638dd02e5c2e791c25e43745f233111b used the original userid for ACL checks located directly in DefineIndex(), but it still adopted the table owner userid for more ACL checks than intended. That broke dump/reload of indexes that refer to an operator class, collation, or exclusion operator in a schema other than "public" or "pg_catalog". Back-patch to v10 (all supported versions), like the earlier commit. Nathan Bossart and Noah Misch Discussion: https://postgr.es/m/[email protected] --- contrib/citext/Makefile | 2 contrib/citext/expected/create_index_acl.out | 81 +++++++++++++++++++++++ contrib/citext/sql/create_index_acl.sql | 82 ++++++++++++++++++++++++ src/backend/commands/indexcmds.c | 92 +++++++++++++++++++++++---- 4 files changed, 244 insertions(+), 13 deletions(-) create mode 100644 contrib/citext/expected/create_index_acl.out create mode 100644 contrib/citext/sql/create_index_acl.sql --- a/contrib/citext/Makefile +++ b/contrib/citext/Makefile @@ -7,7 +7,7 @@ citext--1.0--1.1.sql citext--unpackaged--1.0.sql PGFILEDESC = "citext - case-insensitive character string data type" -REGRESS = citext +REGRESS = create_index_acl citext ifdef USE_PGXS PG_CONFIG = pg_config --- /dev/null +++ b/contrib/citext/expected/create_index_acl.out @@ -0,0 +1,81 @@ +-- Each DefineIndex() ACL check uses either the original userid or the table +-- owner userid; see its header comment. Here, confirm that DefineIndex() +-- uses its original userid where necessary. The test works by creating +-- indexes that refer to as many sorts of objects as possible, with the table +-- owner having as few applicable privileges as possible. (The privileges.sql +-- regress_sro_user tests look for the opposite defect; they confirm that +-- DefineIndex() uses the table owner userid where necessary.) +-- Don't override tablespaces; this version lacks allow_in_place_tablespaces. +BEGIN; +CREATE ROLE regress_minimal; +CREATE SCHEMA s; +CREATE EXTENSION citext SCHEMA s; +-- Revoke all conceivably-relevant ACLs within the extension. The system +-- doesn't check all these ACLs, but this will provide some coverage if that +-- ever changes. +REVOKE ALL ON TYPE s.citext FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_lt(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_le(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_eq(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_ge(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_gt(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_cmp(s.citext, s.citext) FROM PUBLIC; +-- Functions sufficient for making an index column that has the side effect of +-- changing search_path at expression planning time. +CREATE FUNCTION public.setter() RETURNS bool VOLATILE + LANGUAGE SQL AS $$SET search_path = s; SELECT true$$; +CREATE FUNCTION s.const() RETURNS bool IMMUTABLE + LANGUAGE SQL AS $$SELECT public.setter()$$; +CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE + LANGUAGE SQL AS $$SELECT $1$$; +REVOKE ALL ON FUNCTION public.setter() FROM PUBLIC; +REVOKE ALL ON FUNCTION s.const() FROM PUBLIC; +REVOKE ALL ON FUNCTION s.index_this_expr(s.citext, bool) FROM PUBLIC; +-- Even for an empty table, expression planning calls s.const & public.setter. +GRANT EXECUTE ON FUNCTION public.setter() TO regress_minimal; +GRANT EXECUTE ON FUNCTION s.const() TO regress_minimal; +-- Function for index predicate. +CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE + LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$; +REVOKE ALL ON FUNCTION s.index_row_if(s.citext) FROM PUBLIC; +-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions. +GRANT EXECUTE ON FUNCTION s.index_row_if(s.citext) TO regress_minimal; +-- Non-extension, non-function objects. +CREATE COLLATION s.coll (LOCALE="C"); +CREATE TABLE s.x (y s.citext); +ALTER TABLE s.x OWNER TO regress_minimal; +-- Empty-table DefineIndex() +CREATE UNIQUE INDEX u0rows ON s.x USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) + WHERE s.index_row_if(y); +ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) + WHERE (s.index_row_if(y)); +-- Make the table nonempty. +INSERT INTO s.x VALUES ('foo'), ('bar'); +-- If the INSERT runs the planner on index expressions, a search_path change +-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so even +-- under debug_discard_caches, since each index is new-in-transaction. If +-- future work changes a cache lifecycle, this RESET may become necessary. +RESET search_path; +-- For a nonempty table, owner needs permissions throughout ii_Expressions. +GRANT EXECUTE ON FUNCTION s.index_this_expr(s.citext, bool) TO regress_minimal; +CREATE UNIQUE INDEX u2rows ON s.x USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) + WHERE s.index_row_if(y); +ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) + WHERE (s.index_row_if(y)); +-- Shall not find s.coll via search_path, despite the s.const->public.setter +-- call having set search_path=s during expression planning. Suppress the +-- message itself, which depends on the database encoding. +\set VERBOSITY terse +DO $$ +BEGIN +ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=) + WHERE (s.index_row_if(y)); +EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION '%', sqlstate; END$$; +ERROR: 42704 +\set VERBOSITY default +ROLLBACK; --- /dev/null +++ b/contrib/citext/sql/create_index_acl.sql @@ -0,0 +1,82 @@ +-- Each DefineIndex() ACL check uses either the original userid or the table +-- owner userid; see its header comment. Here, confirm that DefineIndex() +-- uses its original userid where necessary. The test works by creating +-- indexes that refer to as many sorts of objects as possible, with the table +-- owner having as few applicable privileges as possible. (The privileges.sql +-- regress_sro_user tests look for the opposite defect; they confirm that +-- DefineIndex() uses the table owner userid where necessary.) + +-- Don't override tablespaces; this version lacks allow_in_place_tablespaces. + +BEGIN; +CREATE ROLE regress_minimal; +CREATE SCHEMA s; +CREATE EXTENSION citext SCHEMA s; +-- Revoke all conceivably-relevant ACLs within the extension. The system +-- doesn't check all these ACLs, but this will provide some coverage if that +-- ever changes. +REVOKE ALL ON TYPE s.citext FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_lt(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_le(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_eq(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_ge(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_gt(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_cmp(s.citext, s.citext) FROM PUBLIC; +-- Functions sufficient for making an index column that has the side effect of +-- changing search_path at expression planning time. +CREATE FUNCTION public.setter() RETURNS bool VOLATILE + LANGUAGE SQL AS $$SET search_path = s; SELECT true$$; +CREATE FUNCTION s.const() RETURNS bool IMMUTABLE + LANGUAGE SQL AS $$SELECT public.setter()$$; +CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE + LANGUAGE SQL AS $$SELECT $1$$; +REVOKE ALL ON FUNCTION public.setter() FROM PUBLIC; +REVOKE ALL ON FUNCTION s.const() FROM PUBLIC; +REVOKE ALL ON FUNCTION s.index_this_expr(s.citext, bool) FROM PUBLIC; +-- Even for an empty table, expression planning calls s.const & public.setter. +GRANT EXECUTE ON FUNCTION public.setter() TO regress_minimal; +GRANT EXECUTE ON FUNCTION s.const() TO regress_minimal; +-- Function for index predicate. +CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE + LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$; +REVOKE ALL ON FUNCTION s.index_row_if(s.citext) FROM PUBLIC; +-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions. +GRANT EXECUTE ON FUNCTION s.index_row_if(s.citext) TO regress_minimal; +-- Non-extension, non-function objects. +CREATE COLLATION s.coll (LOCALE="C"); +CREATE TABLE s.x (y s.citext); +ALTER TABLE s.x OWNER TO regress_minimal; +-- Empty-table DefineIndex() +CREATE UNIQUE INDEX u0rows ON s.x USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) + WHERE s.index_row_if(y); +ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) + WHERE (s.index_row_if(y)); +-- Make the table nonempty. +INSERT INTO s.x VALUES ('foo'), ('bar'); +-- If the INSERT runs the planner on index expressions, a search_path change +-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so even +-- under debug_discard_caches, since each index is new-in-transaction. If +-- future work changes a cache lifecycle, this RESET may become necessary. +RESET search_path; +-- For a nonempty table, owner needs permissions throughout ii_Expressions. +GRANT EXECUTE ON FUNCTION s.index_this_expr(s.citext, bool) TO regress_minimal; +CREATE UNIQUE INDEX u2rows ON s.x USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) + WHERE s.index_row_if(y); +ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) + WHERE (s.index_row_if(y)); +-- Shall not find s.coll via search_path, despite the s.const->public.setter +-- call having set search_path=s during expression planning. Suppress the +-- message itself, which depends on the database encoding. +\set VERBOSITY terse +DO $$ +BEGIN +ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=) + WHERE (s.index_row_if(y)); +EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION '%', sqlstate; END$$; +\set VERBOSITY default +ROLLBACK; --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -70,7 +70,10 @@ Oid relId, char *accessMethodName, Oid accessMethodId, bool amcanorder, - bool isconstraint); + bool isconstraint, + Oid ddl_userid, + int ddl_sec_context, + int *ddl_save_nestlevel); static Oid GetIndexOpClass(List *opclass, Oid attrType, char *accessMethodName, Oid accessMethodId); static char *ChooseIndexName(const char *tabname, Oid namespaceId, @@ -176,8 +179,7 @@ * Compute the operator classes, collations, and exclusion operators for * the new index, so we can test whether it's compatible with the existing * one. Note that ComputeIndexAttrs might fail here, but that's OK: - * DefineIndex would have called this function with the same arguments - * later on, and it would have failed then anyway. + * DefineIndex would have failed later. */ indexInfo = makeNode(IndexInfo); indexInfo->ii_Expressions = NIL; @@ -195,7 +197,7 @@ coloptions, attributeList, exclusionOpNames, relationId, accessMethodName, accessMethodId, - amcanorder, isconstraint); + amcanorder, isconstraint, InvalidOid, 0, NULL); /* Get the soon-obsolete pg_index tuple. */ @@ -288,6 +290,19 @@ * DefineIndex * Creates a new index. * + * This function manages the current userid according to the needs of pg_dump. + * Recreating old-database catalog entries in new-database is fine, regardless + * of which users would have permission to recreate those entries now. That's + * just preservation of state. Running opaque expressions, like calling a + * function named in a catalog entry or evaluating a pg_node_tree in a catalog + * entry, as anyone other than the object owner, is not fine. To adhere to + * those principles and to remain fail-safe, use the table owner userid for + * most ACL checks. Use the original userid for ACL checks reached without + * traversing opaque expressions. (pg_dump can predict such ACL checks from + * catalogs.) Overall, this is a mess. Future DDL development should + * consider offering one DDL command for catalog setup and a separate DDL + * command for steps that run opaque expressions. + * * 'relationId': the OID of the heap relation on which the index is to be * created * 'stmt': IndexStmt describing the properties of the new index. @@ -598,7 +613,8 @@ coloptions, stmt->indexParams, stmt->excludeOpNames, relationId, accessMethodName, accessMethodId, - amcanorder, stmt->isconstraint); + amcanorder, stmt->isconstraint, root_save_userid, + root_save_sec_context, &root_save_nestlevel); /* * Extra checks when creating a PRIMARY KEY index. @@ -706,9 +722,8 @@ /* * Roll back any GUC changes executed by index functions, and keep - * subsequent changes local to this command. It's barely possible that - * some index function changed a behavior-affecting GUC, e.g. xmloption, - * that affects subsequent steps. + * subsequent changes local to this command. This is essential if some + * index function changed a behavior-affecting GUC, e.g. search_path. */ AtEOXact_GUC(false, root_save_nestlevel); root_save_nestlevel = NewGUCNestLevel(); @@ -1063,6 +1078,10 @@ /* * Compute per-index-column information, including indexed column numbers * or index expressions, opclasses, and indoptions. + * + * If the caller switched to the table owner, ddl_userid is the role for ACL + * checks reached without traversing opaque expressions. Otherwise, it's + * InvalidOid, and other ddl_* arguments are undefined. */ static void ComputeIndexAttrs(IndexInfo *indexInfo, @@ -1076,11 +1095,16 @@ char *accessMethodName, Oid accessMethodId, bool amcanorder, - bool isconstraint) + bool isconstraint, + Oid ddl_userid, + int ddl_sec_context, + int *ddl_save_nestlevel) { ListCell *nextExclOp; ListCell *lc; int attn; + Oid save_userid; + int save_sec_context; /* Allocate space for exclusion operator info, if needed */ if (exclusionOpNames) @@ -1096,6 +1120,9 @@ else nextExclOp = NULL; + if (OidIsValid(ddl_userid)) + GetUserIdAndSecContext(&save_userid, &save_sec_context); + /* * process attributeList */ @@ -1190,10 +1217,24 @@ typeOidP[attn] = atttype; /* - * Apply collation override if any + * Apply collation override if any. Use of ddl_userid is necessary + * due to ACL checks therein, and it's safe because collations don't + * contain opaque expressions (or non-opaque expressions). */ if (attribute->collation) + { + if (OidIsValid(ddl_userid)) + { + AtEOXact_GUC(false, *ddl_save_nestlevel); + SetUserIdAndSecContext(ddl_userid, ddl_sec_context); + } attcollation = get_collation_oid(attribute->collation, false); + if (OidIsValid(ddl_userid)) + { + SetUserIdAndSecContext(save_userid, save_sec_context); + *ddl_save_nestlevel = NewGUCNestLevel(); + } + } /* * Check we have a collation iff it's a collatable type. The only @@ -1221,12 +1262,25 @@ collationOidP[attn] = attcollation; /* - * Identify the opclass to use. + * Identify the opclass to use. Use of ddl_userid is necessary due to + * ACL checks therein. This is safe despite opclasses containing + * opaque expressions (specifically, functions), because only + * superusers can define opclasses. */ + if (OidIsValid(ddl_userid)) + { + AtEOXact_GUC(false, *ddl_save_nestlevel); + SetUserIdAndSecContext(ddl_userid, ddl_sec_context); + } classOidP[attn] = GetIndexOpClass(attribute->opclass, atttype, accessMethodName, accessMethodId); + if (OidIsValid(ddl_userid)) + { + SetUserIdAndSecContext(save_userid, save_sec_context); + *ddl_save_nestlevel = NewGUCNestLevel(); + } /* * Identify the exclusion operator, if any. @@ -1240,9 +1294,23 @@ /* * Find the operator --- it must accept the column datatype - * without runtime coercion (but binary compatibility is OK) + * without runtime coercion (but binary compatibility is OK). + * Operators contain opaque expressions (specifically, functions). + * compatible_oper_opid() boils down to oper() and + * IsBinaryCoercible(). PostgreSQL would have security problems + * elsewhere if oper() started calling opaque expressions. */ + if (OidIsValid(ddl_userid)) + { + AtEOXact_GUC(false, *ddl_save_nestlevel); + SetUserIdAndSecContext(ddl_userid, ddl_sec_context); + } opid = compatible_oper_opid(opname, atttype, atttype, false); + if (OidIsValid(ddl_userid)) + { + SetUserIdAndSecContext(save_userid, save_sec_context); + *ddl_save_nestlevel = NewGUCNestLevel(); + } /* * Only allow commutative operators to be used in exclusion --Gfegn71+neI+QTr7 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="CVE-2022-1552_ef792f7856_adapted_for_9.4.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH] CREATE INDEX: use the original userid for more ACL checks. @ 2022-06-25 16:07 Noah Misch <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Noah Misch @ 2022-06-25 16:07 UTC (permalink / raw) Commit a117cebd638dd02e5c2e791c25e43745f233111b used the original userid for ACL checks located directly in DefineIndex(), but it still adopted the table owner userid for more ACL checks than intended. That broke dump/reload of indexes that refer to an operator class, collation, or exclusion operator in a schema other than "public" or "pg_catalog". Back-patch to v10 (all supported versions), like the earlier commit. Nathan Bossart and Noah Misch Discussion: https://postgr.es/m/[email protected] --- contrib/citext/Makefile | 2 contrib/citext/expected/create_index_acl.out | 81 +++++++++++++++++++++++ contrib/citext/sql/create_index_acl.sql | 82 ++++++++++++++++++++++++ src/backend/commands/indexcmds.c | 92 +++++++++++++++++++++++---- 4 files changed, 244 insertions(+), 13 deletions(-) create mode 100644 contrib/citext/expected/create_index_acl.out create mode 100644 contrib/citext/sql/create_index_acl.sql --- a/contrib/citext/Makefile +++ b/contrib/citext/Makefile @@ -7,7 +7,7 @@ citext--1.1--1.0.sql citext--unpackaged--1.0.sql PGFILEDESC = "citext - case-insensitive character string data type" -REGRESS = citext +REGRESS = create_index_acl citext ifdef USE_PGXS PG_CONFIG = pg_config --- /dev/null +++ b/contrib/citext/expected/create_index_acl.out @@ -0,0 +1,81 @@ +-- Each DefineIndex() ACL check uses either the original userid or the table +-- owner userid; see its header comment. Here, confirm that DefineIndex() +-- uses its original userid where necessary. The test works by creating +-- indexes that refer to as many sorts of objects as possible, with the table +-- owner having as few applicable privileges as possible. (The privileges.sql +-- regress_sro_user tests look for the opposite defect; they confirm that +-- DefineIndex() uses the table owner userid where necessary.) +-- Don't override tablespaces; this version lacks allow_in_place_tablespaces. +BEGIN; +CREATE ROLE regress_minimal; +CREATE SCHEMA s; +CREATE EXTENSION citext SCHEMA s; +-- Revoke all conceivably-relevant ACLs within the extension. The system +-- doesn't check all these ACLs, but this will provide some coverage if that +-- ever changes. +REVOKE ALL ON TYPE s.citext FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_lt(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_le(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_eq(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_ge(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_gt(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_cmp(s.citext, s.citext) FROM PUBLIC; +-- Functions sufficient for making an index column that has the side effect of +-- changing search_path at expression planning time. +CREATE FUNCTION public.setter() RETURNS bool VOLATILE + LANGUAGE SQL AS $$SET search_path = s; SELECT true$$; +CREATE FUNCTION s.const() RETURNS bool IMMUTABLE + LANGUAGE SQL AS $$SELECT public.setter()$$; +CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE + LANGUAGE SQL AS $$SELECT $1$$; +REVOKE ALL ON FUNCTION public.setter() FROM PUBLIC; +REVOKE ALL ON FUNCTION s.const() FROM PUBLIC; +REVOKE ALL ON FUNCTION s.index_this_expr(s.citext, bool) FROM PUBLIC; +-- Even for an empty table, expression planning calls s.const & public.setter. +GRANT EXECUTE ON FUNCTION public.setter() TO regress_minimal; +GRANT EXECUTE ON FUNCTION s.const() TO regress_minimal; +-- Function for index predicate. +CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE + LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$; +REVOKE ALL ON FUNCTION s.index_row_if(s.citext) FROM PUBLIC; +-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions. +GRANT EXECUTE ON FUNCTION s.index_row_if(s.citext) TO regress_minimal; +-- Non-extension, non-function objects. +CREATE COLLATION s.coll (LOCALE="C"); +CREATE TABLE s.x (y s.citext); +ALTER TABLE s.x OWNER TO regress_minimal; +-- Empty-table DefineIndex() +CREATE UNIQUE INDEX u0rows ON s.x USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) + WHERE s.index_row_if(y); +ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) + WHERE (s.index_row_if(y)); +-- Make the table nonempty. +INSERT INTO s.x VALUES ('foo'), ('bar'); +-- If the INSERT runs the planner on index expressions, a search_path change +-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so even +-- under debug_discard_caches, since each index is new-in-transaction. If +-- future work changes a cache lifecycle, this RESET may become necessary. +RESET search_path; +-- For a nonempty table, owner needs permissions throughout ii_Expressions. +GRANT EXECUTE ON FUNCTION s.index_this_expr(s.citext, bool) TO regress_minimal; +CREATE UNIQUE INDEX u2rows ON s.x USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) + WHERE s.index_row_if(y); +ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) + WHERE (s.index_row_if(y)); +-- Shall not find s.coll via search_path, despite the s.const->public.setter +-- call having set search_path=s during expression planning. Suppress the +-- message itself, which depends on the database encoding. +\set VERBOSITY terse +DO $$ +BEGIN +ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=) + WHERE (s.index_row_if(y)); +EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION '%', sqlstate; END$$; +ERROR: 42704 +\set VERBOSITY default +ROLLBACK; --- /dev/null +++ b/contrib/citext/sql/create_index_acl.sql @@ -0,0 +1,82 @@ +-- Each DefineIndex() ACL check uses either the original userid or the table +-- owner userid; see its header comment. Here, confirm that DefineIndex() +-- uses its original userid where necessary. The test works by creating +-- indexes that refer to as many sorts of objects as possible, with the table +-- owner having as few applicable privileges as possible. (The privileges.sql +-- regress_sro_user tests look for the opposite defect; they confirm that +-- DefineIndex() uses the table owner userid where necessary.) + +-- Don't override tablespaces; this version lacks allow_in_place_tablespaces. + +BEGIN; +CREATE ROLE regress_minimal; +CREATE SCHEMA s; +CREATE EXTENSION citext SCHEMA s; +-- Revoke all conceivably-relevant ACLs within the extension. The system +-- doesn't check all these ACLs, but this will provide some coverage if that +-- ever changes. +REVOKE ALL ON TYPE s.citext FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_lt(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_le(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_eq(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_ge(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_gt(s.citext, s.citext) FROM PUBLIC; +REVOKE ALL ON FUNCTION s.citext_cmp(s.citext, s.citext) FROM PUBLIC; +-- Functions sufficient for making an index column that has the side effect of +-- changing search_path at expression planning time. +CREATE FUNCTION public.setter() RETURNS bool VOLATILE + LANGUAGE SQL AS $$SET search_path = s; SELECT true$$; +CREATE FUNCTION s.const() RETURNS bool IMMUTABLE + LANGUAGE SQL AS $$SELECT public.setter()$$; +CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE + LANGUAGE SQL AS $$SELECT $1$$; +REVOKE ALL ON FUNCTION public.setter() FROM PUBLIC; +REVOKE ALL ON FUNCTION s.const() FROM PUBLIC; +REVOKE ALL ON FUNCTION s.index_this_expr(s.citext, bool) FROM PUBLIC; +-- Even for an empty table, expression planning calls s.const & public.setter. +GRANT EXECUTE ON FUNCTION public.setter() TO regress_minimal; +GRANT EXECUTE ON FUNCTION s.const() TO regress_minimal; +-- Function for index predicate. +CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE + LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$; +REVOKE ALL ON FUNCTION s.index_row_if(s.citext) FROM PUBLIC; +-- Even for an empty table, CREATE INDEX checks ii_Predicate permissions. +GRANT EXECUTE ON FUNCTION s.index_row_if(s.citext) TO regress_minimal; +-- Non-extension, non-function objects. +CREATE COLLATION s.coll (LOCALE="C"); +CREATE TABLE s.x (y s.citext); +ALTER TABLE s.x OWNER TO regress_minimal; +-- Empty-table DefineIndex() +CREATE UNIQUE INDEX u0rows ON s.x USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) + WHERE s.index_row_if(y); +ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) + WHERE (s.index_row_if(y)); +-- Make the table nonempty. +INSERT INTO s.x VALUES ('foo'), ('bar'); +-- If the INSERT runs the planner on index expressions, a search_path change +-- survives. As of 2022-06, the INSERT reuses a cached plan. It does so even +-- under debug_discard_caches, since each index is new-in-transaction. If +-- future work changes a cache lifecycle, this RESET may become necessary. +RESET search_path; +-- For a nonempty table, owner needs permissions throughout ii_Expressions. +GRANT EXECUTE ON FUNCTION s.index_this_expr(s.citext, bool) TO regress_minimal; +CREATE UNIQUE INDEX u2rows ON s.x USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_ops) + WHERE s.index_row_if(y); +ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=) + WHERE (s.index_row_if(y)); +-- Shall not find s.coll via search_path, despite the s.const->public.setter +-- call having set search_path=s during expression planning. Suppress the +-- message itself, which depends on the database encoding. +\set VERBOSITY terse +DO $$ +BEGIN +ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree + ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=) + WHERE (s.index_row_if(y)); +EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION '%', sqlstate; END$$; +\set VERBOSITY default +ROLLBACK; --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -65,7 +65,10 @@ Oid relId, char *accessMethodName, Oid accessMethodId, bool amcanorder, - bool isconstraint); + bool isconstraint, + Oid ddl_userid, + int ddl_sec_context, + int *ddl_save_nestlevel); static Oid GetIndexOpClass(List *opclass, Oid attrType, char *accessMethodName, Oid accessMethodId); static char *ChooseIndexName(const char *tabname, Oid namespaceId, @@ -168,8 +171,7 @@ * Compute the operator classes, collations, and exclusion operators for * the new index, so we can test whether it's compatible with the existing * one. Note that ComputeIndexAttrs might fail here, but that's OK: - * DefineIndex would have called this function with the same arguments - * later on, and it would have failed then anyway. + * DefineIndex would have failed later. */ indexInfo = makeNode(IndexInfo); indexInfo->ii_Expressions = NIL; @@ -187,7 +189,7 @@ coloptions, attributeList, exclusionOpNames, relationId, accessMethodName, accessMethodId, - amcanorder, isconstraint); + amcanorder, isconstraint, InvalidOid, 0, NULL); /* Get the soon-obsolete pg_index tuple. */ @@ -280,6 +282,19 @@ * DefineIndex * Creates a new index. * + * This function manages the current userid according to the needs of pg_dump. + * Recreating old-database catalog entries in new-database is fine, regardless + * of which users would have permission to recreate those entries now. That's + * just preservation of state. Running opaque expressions, like calling a + * function named in a catalog entry or evaluating a pg_node_tree in a catalog + * entry, as anyone other than the object owner, is not fine. To adhere to + * those principles and to remain fail-safe, use the table owner userid for + * most ACL checks. Use the original userid for ACL checks reached without + * traversing opaque expressions. (pg_dump can predict such ACL checks from + * catalogs.) Overall, this is a mess. Future DDL development should + * consider offering one DDL command for catalog setup and a separate DDL + * command for steps that run opaque expressions. + * * 'relationId': the OID of the heap relation on which the index is to be * created * 'stmt': IndexStmt describing the properties of the new index. @@ -581,7 +596,8 @@ coloptions, stmt->indexParams, stmt->excludeOpNames, relationId, accessMethodName, accessMethodId, - amcanorder, stmt->isconstraint); + amcanorder, stmt->isconstraint, root_save_userid, + root_save_sec_context, &root_save_nestlevel); /* * Extra checks when creating a PRIMARY KEY index. @@ -639,9 +655,8 @@ /* * Roll back any GUC changes executed by index functions, and keep - * subsequent changes local to this command. It's barely possible that - * some index function changed a behavior-affecting GUC, e.g. xmloption, - * that affects subsequent steps. + * subsequent changes local to this command. This is essential if some + * index function changed a behavior-affecting GUC, e.g. search_path. */ AtEOXact_GUC(false, root_save_nestlevel); root_save_nestlevel = NewGUCNestLevel(); @@ -996,6 +1011,10 @@ /* * Compute per-index-column information, including indexed column numbers * or index expressions, opclasses, and indoptions. + * + * If the caller switched to the table owner, ddl_userid is the role for ACL + * checks reached without traversing opaque expressions. Otherwise, it's + * InvalidOid, and other ddl_* arguments are undefined. */ static void ComputeIndexAttrs(IndexInfo *indexInfo, @@ -1009,11 +1028,16 @@ char *accessMethodName, Oid accessMethodId, bool amcanorder, - bool isconstraint) + bool isconstraint, + Oid ddl_userid, + int ddl_sec_context, + int *ddl_save_nestlevel) { ListCell *nextExclOp; ListCell *lc; int attn; + Oid save_userid; + int save_sec_context; /* Allocate space for exclusion operator info, if needed */ if (exclusionOpNames) @@ -1029,6 +1053,9 @@ else nextExclOp = NULL; + if (OidIsValid(ddl_userid)) + GetUserIdAndSecContext(&save_userid, &save_sec_context); + /* * process attributeList */ @@ -1123,10 +1150,24 @@ typeOidP[attn] = atttype; /* - * Apply collation override if any + * Apply collation override if any. Use of ddl_userid is necessary + * due to ACL checks therein, and it's safe because collations don't + * contain opaque expressions (or non-opaque expressions). */ if (attribute->collation) + { + if (OidIsValid(ddl_userid)) + { + AtEOXact_GUC(false, *ddl_save_nestlevel); + SetUserIdAndSecContext(ddl_userid, ddl_sec_context); + } attcollation = get_collation_oid(attribute->collation, false); + if (OidIsValid(ddl_userid)) + { + SetUserIdAndSecContext(save_userid, save_sec_context); + *ddl_save_nestlevel = NewGUCNestLevel(); + } + } /* * Check we have a collation iff it's a collatable type. The only @@ -1154,12 +1195,25 @@ collationOidP[attn] = attcollation; /* - * Identify the opclass to use. + * Identify the opclass to use. Use of ddl_userid is necessary due to + * ACL checks therein. This is safe despite opclasses containing + * opaque expressions (specifically, functions), because only + * superusers can define opclasses. */ + if (OidIsValid(ddl_userid)) + { + AtEOXact_GUC(false, *ddl_save_nestlevel); + SetUserIdAndSecContext(ddl_userid, ddl_sec_context); + } classOidP[attn] = GetIndexOpClass(attribute->opclass, atttype, accessMethodName, accessMethodId); + if (OidIsValid(ddl_userid)) + { + SetUserIdAndSecContext(save_userid, save_sec_context); + *ddl_save_nestlevel = NewGUCNestLevel(); + } /* * Identify the exclusion operator, if any. @@ -1173,9 +1227,23 @@ /* * Find the operator --- it must accept the column datatype - * without runtime coercion (but binary compatibility is OK) + * without runtime coercion (but binary compatibility is OK). + * Operators contain opaque expressions (specifically, functions). + * compatible_oper_opid() boils down to oper() and + * IsBinaryCoercible(). PostgreSQL would have security problems + * elsewhere if oper() started calling opaque expressions. */ + if (OidIsValid(ddl_userid)) + { + AtEOXact_GUC(false, *ddl_save_nestlevel); + SetUserIdAndSecContext(ddl_userid, ddl_sec_context); + } opid = compatible_oper_opid(opname, atttype, atttype, false); + if (OidIsValid(ddl_userid)) + { + SetUserIdAndSecContext(save_userid, save_sec_context); + *ddl_save_nestlevel = NewGUCNestLevel(); + } /* * Only allow commutative operators to be used in exclusion --Gfegn71+neI+QTr7-- ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2022-06-25 16:07 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-01-07 17:44 Re: Disallow quorum uncommitted (with synchronous standbys) txns in logical replication subscribers SATYANARAYANA NARLAPURAM <[email protected]> 2022-01-07 18:23 ` Bossart, Nathan <[email protected]> 2022-01-11 08:30 ` Kyotaro Horiguchi <[email protected]> 2022-06-25 16:07 [PATCH] CREATE INDEX: use the original userid for more ACL checks. Noah Misch <[email protected]> 2022-06-25 16:07 [PATCH] CREATE INDEX: use the original userid for more ACL checks. Noah Misch <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox