Re: Support logical replication of DDLs

Hi,

I'm working on test coverage for the DDL deparser using the
test_ddl_deparse_regress test module. We’re focusing on table commands
as the first step in providing test coverage as proposed in [1], I
have constructed unit test cases by testing each clause in the syntax
and complex test cases by combining multiple clauses together. I found
about 20 test failures and they are marked TOFIX in the test script,
Zane is working on fixing these test cases. I’ve added coverage tests
for CREATE TABLE, table/column constraints and most commands for ALTER
TABLE (except ENABLE/DISABLE TRIGGER, OWNER).

I’ve also made the testing module more convenient for debugging by
printing the error message and saving the dump diffs. The testing
module will first compare the generated JSON blobs and reformed SQL
commands with SQL tests. After SQL tests pass, it executes the
original SQL commands on a node (called pub node) and output the
reformed SQL commands to /tmp_check/ddl directory. Then it executes
the reformed SQL commands on a separate node (called sub node), if the
execution is successful, it will compare the dumped results between
pub and sub, the difference is output to /tmp_check/dumps directory.
Test is successful if there is no dump difference. You can read more
about the testing module in the readme file. Patch
0004-Introduce-the-test_ddl_deparse_regress-test-modu.patch is
modified for the above changes

For the next step, I'm going to add more test cases for CREATE TABLE
AS commands and the dependent commands for TABLE related commands like
CREATE INDEX, CREATE SCEQUENCE, etc. Let me know if you have any
feedback about this testing plan. Thanks!

Regards,
Runqi Tian

[1] https://www.postgresql.org/message-id/CAAD30U%2BfqaaD6533-eiaWVHpUaBNBCEvqyXOT_ow1B--Aa_jOQ%40mail.gmail.com

On Fri, Mar 10, 2023 at 5:29 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, 6 Mar 2023 at 18:43, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Mon, 6 Mar 2023 at 12:04, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Feb 15, 2023 at 3:33 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > >
> > > > > >
> > > > > > 9.
> > > > > > +
> > > > > > +/*
> > > > > > + * Append the parenthesized arguments of the given pg_proc row into the output
> > > > > > + * buffer. force_qualify indicates whether to schema-qualify type names
> > > > > > + * regardless of visibility.
> > > > > > + */
> > > > > > +static void
> > > > > > +format_procedure_args_internal(Form_pg_proc procform, StringInfo buf,
> > > > > > + bool force_qualify)
> > > > > > +{
> > > > > > + int i;
> > > > > > + char* (*func[2])(Oid) = {format_type_be, format_type_be_qualified};
> > > > > > +
> > > > > > + appendStringInfoChar(buf, '(');
> > > > > > + for (i = 0; i < procform->pronargs; i++)
> > > > > > + {
> > > > > > + Oid thisargtype = procform->proargtypes.values[i];
> > > > > > + char *argtype = NULL;
> > > > > > +
> > > > > > + if (i > 0)
> > > > > > + appendStringInfoChar(buf, ',');
> > > > > > +
> > > > > > + argtype = func[force_qualify](thisargtype);
> > > > > > + appendStringInfoString(buf, argtype);
> > > > > > + pfree(argtype);
> > > > > > + }
> > > > > > + appendStringInfoChar(buf, ')');
> > > > > > +}
> > > > > >
> > > > > > 9b.
> > > > > > I understand why this function was put here beside the other static
> > > > > > functions in "Support Routines" but IMO it really belongs nearby (i.e.
> > > > > > directly above) the only caller (format_procedure_args). Keeping both
> > > > > > those functional together will improve the readability of both, and
> > > > > > will also remove the need to have the static forward declaration.
> > > > > >
> > > >
> > > > There was no reply for 9b. Was it accidentally overlooked, or just
> > > > chose not to do it?
> > >
> > > Fixed this. Moved the function up and removed the forward declaration.
> > >
> > > On Wed, Feb 15, 2023 at 3:00 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > >
> > > > On Sat, Feb 11, 2023 at 3:21 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Thu, 9 Feb 2023 at 03:47, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > > > >
> > > > > > Hi Vignesh, thanks for addressing my v63-0002 review comments.
> > > > > >
> > > > > > I confirmed most of the changes. Below is a quick follow-up for the
> > > > > > remaining ones.
> > > > > >
> > > > > > On Mon, Feb 6, 2023 at 10:32 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > > > >
> > > > > > > On Mon, 6 Feb 2023 at 06:47, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > > > > > >
> > > > > > ...
> > > > > > > >
> > > > > > > > 8.
> > > > > > > > + value = findJsonbValueFromContainer(container, JB_FOBJECT, &key);
> > > > > > > >
> > > > > > > > Should the code be checking or asserting value is not NULL?
> > > > > > > >
> > > > > > > > (IIRC I asked this a long time ago - sorry if it was already answered)
> > > > > > > >
> > > > > > >
> > > > > > > Yes, this was already answered by Zheng, quoting as "The null checking
> > > > > > > for value is done in the upcoming call of expand_one_jsonb_element()."
> > > > > > > in [1]
> > > > > >
> > > > > > Thanks for the info. I saw that Zheng-san only wrote it is handled in
> > > > > > the “upcoming call of expand_one_jsonb_element”, but I don’t know if
> > > > > > that is sufficient. For example, if the execution heads down the other
> > > > > > path (expand_jsonb_array) with a NULL jsonarr then it going to crash,
> > > > > > isn't it? So I still think some change may be needed here.
> > > > >
> > > > > Added an Assert for this.
> > > > >
> > > >
> > > > Was this a correct change to make here?
> > > >
> > > > IIUC this Assert is now going to intercept both cases including the
> > > > expand_one_jsonb_element() which previously would have thrown a proper
> > > > ERROR.
> > > >
> > > Fixed this. Added an error check in expand_jsonb_array() as well.
> > >
> > > Changes are in patch 1 and patch 2
> >
> > Few comments:
> > 1) The following statement crashes:
> > CREATE TABLE itest7b (a int);
> > CREATE TABLE itest7c (a int GENERATED ALWAYS AS IDENTITY) INHERITS (itest7b);
> > #0 0x0000559018aff927 in RangeVarGetRelidExtended (relation=0x0,
> > lockmode=0, flags=0, callback=0x0, callback_arg=0x0) at
> > namespace.c:255
> > #1 0x0000559018be09dc in deparse_ColumnDef (relation=0x7f3e917abba8,
> > dpcontext=0x55901a792668, composite=false, coldef=0x55901a77d758,
> > is_alter=false, exprs=0x0) at ddl_deparse.c:1657
> > #2 0x0000559018be2271 in deparse_TableElements
> > (relation=0x7f3e917abba8, tableElements=0x55901a77d708,
> > dpcontext=0x55901a792668, typed=false, composite=false) at
> > ddl_deparse.c:2460
> > #3 0x0000559018be2b89 in deparse_CreateStmt (objectId=16420,
> > parsetree=0x55901a77d5f8) at ddl_deparse.c:2722
> > #4 0x0000559018bf72c3 in deparse_simple_command (cmd=0x55901a77d590,
> > include_owner=0x7ffe4e611234) at ddl_deparse.c:10019
> > #5 0x0000559018bf7563 in deparse_utility_command (cmd=0x55901a77d590,
> > include_owner=true, verbose_mode=false) at ddl_deparse.c:10122
> > #6 0x0000559018eb650d in publication_deparse_ddl_command_end
> > (fcinfo=0x7ffe4e6113f0) at ddltrigger.c:203
>
> Fixed
>
> > 2) invalid type storage error:
> > CREATE TYPE myvarchar;
> >
> > CREATE FUNCTION myvarcharin(cstring, oid, integer) RETURNS myvarchar
> > LANGUAGE internal IMMUTABLE PARALLEL SAFE STRICT AS 'varcharin';
> >
> > CREATE FUNCTION myvarcharout(myvarchar) RETURNS cstring
> > LANGUAGE internal IMMUTABLE PARALLEL SAFE STRICT AS 'varcharout';
> >
> > CREATE FUNCTION myvarcharsend(myvarchar) RETURNS bytea
> > LANGUAGE internal STABLE PARALLEL SAFE STRICT AS 'varcharsend';
> >
> > CREATE FUNCTION myvarcharrecv(internal, oid, integer) RETURNS myvarchar
> > LANGUAGE internal STABLE PARALLEL SAFE STRICT AS 'varcharrecv';
> >
> > CREATE TYPE myvarchar (
> > input = myvarcharin,
> > output = myvarcharout,
> > alignment = integer,
> > storage = main
> > );
> >
> > -- want to check updating of a domain over the target type, too
> > CREATE DOMAIN myvarchardom AS myvarchar;
> >
> > ALTER TYPE myvarchar SET (storage = extended);
>
> Fixed
>
> > 3) invalid type option send
> > ALTER TYPE myvarchar SET (
> > send = myvarcharsend,
> > receive = myvarcharrecv,
> > typmod_in = varchartypmodin,
> > typmod_out = varchartypmodout,
> > -- these are bogus, but it's safe as long as we don't use the type:
> > analyze = ts_typanalyze,
> > subscript = raw_array_subscript_handler
> > );
>
> Fixed
>
> > 4) There are some unsupported alter table subtype:
> > CREATE FOREIGN DATA WRAPPER dummy;
> > CREATE SERVER s0 FOREIGN DATA WRAPPER dummy;
> > CREATE FOREIGN TABLE ft1 (
> > c1 integer OPTIONS ("param 1" 'val1') NOT NULL,
> > c2 text OPTIONS (param2 'val2', param3 'val3') CHECK (c2 <> ''),
> > c3 date,
> > CHECK (c3 BETWEEN '1994-01-01'::date AND '1994-01-31'::date)
> > ) SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value');
>
> Fixed
>
> > 5) similarly in case of alter foreign table:
> > ALTER FOREIGN TABLE ft1 ADD COLUMN c10 integer OPTIONS (p1 'v1');
>
> Fixed
>
> > 6) Few whitespace errors:
> > Applying: Infrastructure to support DDL deparsing.
> > .git/rebase-apply/patch:486: indent with spaces.
> > bool force_qualify)
> > .git/rebase-apply/patch:488: indent with spaces.
> > int i;
> > .git/rebase-apply/patch:489: indent with spaces.
> > char* (*func[2])(Oid) = {format_type_be, format_type_be_qualified};
> > .git/rebase-apply/patch:491: indent with spaces.
> > appendStringInfoChar(buf, '(');
> > .git/rebase-apply/patch:492: indent with spaces.
> > for (i = 0; i < procform->pronargs; i++)
> > warning: squelched 10 whitespace errors
> > warning: 15 lines add whitespace errors.
>
> These were already fixed
>
> > 7) Alter foreign table rename not handled:
> > ALTER FOREIGN TABLE foreign_schema.ft1 RENAME TO foreign_table_1;
>
> Fixed
>
> Attached v77 version patch has the fixes for the same.
> Patches 0002 and 0003 were modified to fix the above issues.
>
> Regards,
> Vignesh