Re: [HACKERS] [PATCH] WIP Add ALWAYS DEFERRED option for constraints - Mailing list pgsql-hackers

On Tue, Jun 26, 2018 at 04:54:13PM -0400, Robbie Harwood wrote:
> Nico Williams <nico@cryptonector.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><iteration count></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
--