Re: Support logical replication of DDLs

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Ajin Cherian <itsajin(at)gmail(dot)com>
Cc: Peter Smith <smithpb2250(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Zheng Li <zhengli10(at)gmail(dot)com>, li jie <ggysxcq(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Japin Li <japinli(at)hotmail(dot)com>, rajesh singarapu <rajesh(dot)rs0541(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Support logical replication of DDLs
Date: 2023-02-09 10:55:22
Message-ID: 20230209105522.h6fsm6u3mhn2n6ie@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-general pgsql-hackers

I happened to notice that MINVFUNC in 0003 displays like this
"fmt": "MINVFUNC==%{type}T",
in some cases; this appears in the JSON that's emitted by the regression
tests at some point. How can we detect this kind of thing, so that
these mistakes become self-evident? I thought the intention of the
regress module was to run the deparsed code, so the syntax error should
have become obvious.

...

Oh, I see the problem. There are two 'fmt' lines for that clause (and
many others), one of which is used when the clause is not present. So
there's never a syntax error, because this one never expands other than
to empty.

AFAICS this defeats the purpose of the 'present' field. I mean, if the
clause is never to deparse, then why have it there in the first place?
If we want to have it, then it has to be correct.

I think we should design the code to avoid the repetition, because that
has an inherent risk of typo bugs and such. Maybe we should set forth
policy that each 'fmt' string should appear in the source code only
once. So instead of this

+ /* MINVFUNC */
+ if (OidIsValid(agg->aggminvtransfn))
+ tmp = new_objtree_VA("MINVFUNC=%{type}T", 1,
+ "type", ObjTypeObject,
+ new_objtree_for_qualname_id(ProcedureRelationId,
+ agg->aggminvtransfn));
+ else
+ {
+ tmp = new_objtree("MINVFUNC==%{type}T");
+ append_bool_object(tmp, "present", false);
+ }

we would have something like

tmp = new_objtree("MINVFUNC=%{type}T");
if (OidIsValid(agg->aggminvtransfn))
{
append_bool_object(tmp, "present", true);
append...(tmp, "type", new_objtree_for_qualname_id(ProcedureRelationId, agg->aggminvtransfn));
}
else
{
append_bool_object(tmp, "present", false);
}

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"No necesitamos banderas
No reconocemos fronteras" (Jorge González)

In response to

Responses

Browse pgsql-general by date

  From Date Subject
Next Message Jehan-Guillaume de Rorthais 2023-02-09 13:49:47 Re: Question regarding UTF-8 data and "C" collation on definition of field of table
Previous Message Ajin Cherian 2023-02-09 09:55:17 Re: Support logical replication of DDLs

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2023-02-09 11:01:12 Re: [PATCH] Compression dictionaries for JSONB
Previous Message Aleksander Alekseev 2023-02-09 10:50:57 Re: [PATCH] Compression dictionaries for JSONB