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

On Tue, Jun 26, 2018 at 04:54:13PM -0400, Robbie Harwood wrote:
> 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.

Thanks. Replies below.

> > All tests (make check) pass.
>
> Thank you for adding tests!

Well, yeah :)

> > 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'.

Oh yes, good catch.

> > + <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?

Oy, nothing. I think that's a cut-n-paste error.

I'll remove it.

> > }
> >
> > /* 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.)

I had found the answer ("yes") in the history of this file but forgot to
remove the comment while rebasing. I'll remove the comment.

> > 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.

Ah, this became unused during the rebase. I'll remove it.

> > 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?

Maybe! :) I couldn't quite figure out what it should be. There
doesn't seem to be a consistent style of wrapping.

> > 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.)

Oy, I forgot about these. The number was a bread crumb I forgot to
cleanup :( So sorry about that.

> > 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.)

OK.

> > + {
> > + 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

Thank you. I'll rebase, update, and post a new patch soon.

Nico
--