Re: [HACKERS] [PATCH] WIP Add ALWAYS DEFERRED option for constraints

Nico Williams <nico(at)cryptonector(dot)com> writes:

> [Re-send; first attempt appears to have hit /dev/null somewhere. My
> apologies if you get two copies.]
>
> I've finally gotten around to rebasing this patch and making the change
> that was requested, which was: merge the now-would-be-three deferral-
> related bool columns in various pg_catalog tables into one char column.
>
> Instead of (deferrable, initdeferred, alwaysdeferred), now there is just
> (deferral).

This design seems correct to me. I have a couple questions inline, and
some nits to go with them.

> All tests (make check) pass.

Thank you for adding tests!

> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
> index 3ed9021..e82e39b 100644
> --- a/doc/src/sgml/catalogs.sgml
> +++ b/doc/src/sgml/catalogs.sgml
> @@ -2239,17 +2239,15 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
> </row>
>
> <row>
> - <entry><structfield>condeferrable</structfield></entry>
> - <entry><type>bool</type></entry>
> - <entry></entry>
> - <entry>Is the constraint deferrable?</entry>
> - </row>
> -
> - <row>
> - <entry><structfield>condeferred</structfield></entry>
> - <entry><type>bool</type></entry>
> + <entry><structfield>condeferral</structfield></entry>
> + <entry><type>char</type></entry>
> <entry></entry>
> - <entry>Is the constraint deferred by default?</entry>
> + <entry>Constraint deferral option:
> + <literal>a</literal> = always deferred,
> + <literal>d</literal> = deferrable,
> + <literal>d</literal> = deferrable initially deferred,

From the rest of the code, I think this is supposed to be 'i', not 'd'.

> + <literal>n</literal> = not deferrable
> + </entry>
> </row>
>
> <row>

> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
> index 8b276bc..795a7a9 100644
> --- a/src/backend/catalog/index.c
> +++ b/src/backend/catalog/index.c
> @@ -1070,6 +1070,7 @@ index_create(Relation heapRelation,
>
> recordDependencyOn(&myself, &referenced, deptype);
> }
> + Assert((flags & INDEX_CREATE_ADD_CONSTRAINT) == 0);

What does this ensure, and why is it in this part of the code? We're in
the `else` branch here - isn't this always the case (i.e., Assert can
never fire), since `flags` isn't manipulated in this block?

> }
>
> /* Store dependency on parent index, if any */

> diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
> index f4e69f4..bde6199 100644
> --- a/src/backend/catalog/information_schema.sql
> +++ b/src/backend/catalog/information_schema.sql
> @@ -891,10 +891,14 @@ CREATE VIEW domain_constraints AS
> CAST(current_database() AS sql_identifier) AS domain_catalog,
> CAST(n.nspname AS sql_identifier) AS domain_schema,
> CAST(t.typname AS sql_identifier) AS domain_name,
> - CAST(CASE WHEN condeferrable THEN 'YES' ELSE 'NO' END
> + CAST(CASE WHEN condeferral = 'n' THEN 'NO' ELSE 'YES' END
> AS yes_or_no) AS is_deferrable,
> - CAST(CASE WHEN condeferred THEN 'YES' ELSE 'NO' END
> + CAST(CASE WHEN condeferral = 'i' OR condeferral = 'a' THEN 'YES' ELSE 'NO' END
> AS yes_or_no) AS initially_deferred
> + /*
> + * XXX Can we add is_always_deferred here? Are there
> + * standards considerations?
> + */

I'm not familiar enough to speak to this. Hopefully someone else can.
Absent other input, I think we should add is_always_deferred. (And if
we were building this today, probably just expose a single character
instead of three booleans.)

> FROM pg_namespace rs, pg_namespace n, pg_constraint con, pg_type t
> WHERE rs.oid = con.connamespace
> AND n.oid = t.typnamespace

> diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
> index 57519fe..41dc6a4 100644
> --- a/src/backend/commands/trigger.c
> +++ b/src/backend/commands/trigger.c
> @@ -3632,6 +3627,7 @@ typedef struct AfterTriggerSharedData
> TriggerEvent ats_event; /* event type indicator, see trigger.h */
> Oid ats_tgoid; /* the trigger's ID */
> Oid ats_relid; /* the relation it's on */
> + bool ats_alwaysdeferred; /* whether this can be deferred */

"Whether this must be deferred"? Also, I'm not sure what this is for -
it doesn't seem to be used anywhere.

> CommandId ats_firing_id; /* ID for firing cycle */
> struct AfterTriggersTableData *ats_table; /* transition table access */
> } AfterTriggerSharedData;

> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index 90dfac2..dab721a 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -184,8 +185,8 @@ static void SplitColQualList(List *qualList,
> List **constraintList, CollateClause **collClause,
> core_yyscan_t yyscanner);
> static void processCASbits(int cas_bits, int location, const char *constrType,
> - bool *deferrable, bool *initdeferred, bool *not_valid,
> - bool *no_inherit, core_yyscan_t yyscanner);
> + char *deferral,
> + bool *not_valid, bool *no_inherit, core_yyscan_t yyscanner);

Can you fix the wrapping on this?

> static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>
> %}
> @@ -5538,17 +5568,24 @@ ConstraintAttributeSpec:
> int newspec = $1 | $2;
>
> /* special message for this case */
> - if ((newspec & (CAS_NOT_DEFERRABLE | CAS_INITIALLY_DEFERRED)) == (CAS_NOT_DEFERRABLE | CAS_INITIALLY_DEFERRED))
> + if ((newspec & CAS_NOT_DEFERRABLE) &&
> + (newspec & (CAS_INITIALLY_DEFERRED | CAS_ALWAYS_DEFERRED)))
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("constraint declared INITIALLY DEFERRED must be DEFERRABLE"),
> parser_errposition(@2)));
> /* generic message for other conflicts */
> + if ((newspec & CAS_ALWAYS_DEFERRED) &&
> + (newspec & (CAS_INITIALLY_IMMEDIATE)))
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("conflicting constraint properties 1"),
> + parser_errposition(@2)));
> if ((newspec & (CAS_NOT_DEFERRABLE | CAS_DEFERRABLE)) == (CAS_NOT_DEFERRABLE | CAS_DEFERRABLE) ||
> (newspec & (CAS_INITIALLY_IMMEDIATE | CAS_INITIALLY_DEFERRED)) == (CAS_INITIALLY_IMMEDIATE | CAS_INITIALLY_DEFERRED))
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> - errmsg("conflicting constraint properties"),
> + errmsg("conflicting constraint properties 2"),

I'd prefer you just repeat the message (or make them more situationally
descriptive), rather than appending a number. (Repeating error messages
is in keeping with the style here.)

> parser_errposition(@2)));
> $$ = newspec;
> }
> @@ -16197,34 +16234,41 @@ SplitColQualList(List *qualList,
> */
> static void
> processCASbits(int cas_bits, int location, const char *constrType,
> - bool *deferrable, bool *initdeferred, bool *not_valid,
> - bool *no_inherit, core_yyscan_t yyscanner)
> + char *deferral,
> + bool *not_valid, bool *no_inherit, core_yyscan_t yyscanner)

Line wrapping?

> {
> /* defaults */
> - if (deferrable)
> - *deferrable = false;
> - if (initdeferred)
> - *initdeferred = false;
> + if (deferral)
> + *deferral = 'n';
> if (not_valid)
> *not_valid = false;
>
> - if (cas_bits & (CAS_DEFERRABLE | CAS_INITIALLY_DEFERRED))
> + if (cas_bits & CAS_ALWAYS_DEFERRED)
> {
> - if (deferrable)
> - *deferrable = true;
> + if (deferral)
> + *deferral = 'a';
> else
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> /* translator: %s is CHECK, UNIQUE, or similar */
> - errmsg("%s constraints cannot be marked DEFERRABLE",
> + errmsg("%s constraints cannot be marked ALWAYS DEFERRED",
> constrType),
> parser_errposition(location)));
> - }
> -
> - if (cas_bits & CAS_INITIALLY_DEFERRED)
> + } else if (cas_bits & CAS_INITIALLY_DEFERRED)

Style on this file doesn't cuddle `else`. (i.e., `else if (cond)` gets its own
line without any braces on it.)

> + {
> + if (deferral)
> + *deferral = 'i';
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + /* translator: %s is CHECK, UNIQUE, or similar */
> + errmsg("%s constraints cannot be marked INITIALLY DEFERRED",
> + constrType),
> + parser_errposition(location)));
> + } else if (cas_bits & CAS_DEFERRABLE)
> {
> - if (initdeferred)
> - *initdeferred = true;
> + if (deferral)
> + *deferral = 'd';
> else
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),

Thanks,
--Robbie