On Fri, Oct 8, 2021 at 4:34 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Thu, Oct 7, 2021 at 5:19 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > On Wed, Oct 6, 2021 at 11:12 AM vignesh C <vignes...@gmail.com> wrote:
> > >
> > > Attached v37 patch has the changes for the same.
> > >
> >
> > Few comments:
> > ==============
> >
>
> Few more comments:
> ====================
> v37-0002-Client-side-changes-to-support-FOR-ALL-TABLES-IN
> 1.
> + else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
> "TABLES", "IN", "SCHEMA"))
> + COMPLETE_WITH_QUERY(Query_for_list_of_schemas
> + " UNION SELECT 'CURRENT_SCHEMA' "
> + "UNION SELECT 'WITH ('");
>
> What is the need to display WITH here? It will be displayed after
> Schema name with the below rule:
> + else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
> "TABLES", "IN", "SCHEMA", MatchAny))
> + COMPLETE_WITH("WITH (");
Removed it.
> 2. It seems tab-completion happens incorrectly for the below case:
> create publication pub for all tables in schema s1,
>
> If I press the tab after above, it completes with below which is wrong
> because it will lead to incorrect syntax.
>
> create publication pub for all tables in schema s1, WITH (
Modified
> v37-0003-Tests-for-FOR-ALL-TABLES-IN-SCHEMA-publication
> 3.
> --- a/src/bin/pg_dump/t/002_pg_dump.pl
> +++ b/src/bin/pg_dump/t/002_pg_dump.pl
> ..
> + 'ALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test' => {
> + create_order => 51,
> + create_sql =>
> + 'ALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test;',
> + regexp => qr/^
> + \QALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test;\E
> + /xm,
> + like => { %full_runs, section_post_data => 1, },
> + unlike => { exclude_dump_test_schema => 1, },
>
> In this test, won't it exclude the schema dump_test because of unlike?
> If so, then we don't have coverage for "ALL Tables In Schema" except
> for public schema?
I noticed that the tap test framework runs pg_dump tests with various
combinations, these tests will be covered in the like tests as Tang
suggested at [1]. In the exclude_dump_test_schema since this sql will
not be present, exclude_dump_test_schema should be added in the
unlike option, as the validation will fail for exclude-schema option
which will be run with the following command:
pg_dump --no-sync
--file=/home/vignesh/postgres/src/bin/pg_dump/tmp_check/tmp_test_4i8F/exclude_dump_test_schema.sql
--exclude-schema=dump_test postgres
> 4.
> --- a/src/test/regress/expected/publication.out
> +++ b/src/test/regress/expected/publication.out
> ..
> +-- fail - can't add schema to for all tables publication
> +ALTER PUBLICATION testpub_foralltables ADD ALL TABLES IN SCHEMA pub_test;
>
> In the above and all similar comments, it is better to either quote
> 'for all tables' or write in CAPS FOR ALL TABLE or both 'FOR ALL
> TABLE'.
Modified
> 5.
> +\dRp+ testpub1_forschema
> + Publication testpub1_forschema
> + Owner | All tables | Inserts | Updates | Deletes
> | Truncates | Via root
> +--------------------------+------------+---------+---------+---------+-----------+----------
> + regress_publication_user | f | t | t | t
> | t | f
> +Tables from schemas:
> + "pub_test1"
> +
> +SELECT p.pubname FROM pg_catalog.pg_publication p,
> pg_catalog.pg_namespace n, pg_catalog.pg_publication_namespace pn
> WHERE n.oid = pn.pnnspid AND p.oid = pn.pnpubid AND n.nspname =
> 'pub_test1' ORDER BY 1;
> + pubname
> +--------------------
> + testpub1_forschema
> +(1 row)
>
> I don't think in the above and similar tests, we need to separately
> check the presence of publication via Select query, if we have tested
> it via psql command. Let's try to keep the meaningful tests.
Removed it.
> 6.
> +INSERT INTO pub_test1.tbl VALUES(1, 'test');
> +-- fail
> +UPDATE pub_test1.tbl SET id = 2;
> +ERROR: cannot update table "tbl" because it does not have a replica
> identity and publishes updates
> +HINT: To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
> +ALTER PUBLICATION testpub1_forschema DROP ALL TABLES IN SCHEMA pub_test1;
> +-- success
> +UPDATE pub_test1.tbl SET id = 2;
> +ALTER PUBLICATION testpub1_forschema SET ALL TABLES IN SCHEMA pub_test1;
> +-- fail
> +UPDATE pub_test1.tbl SET id = 2;
> +ERROR: cannot update table "tbl" because it does not have a replica
> identity and publishes updates
> +HINT: To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
> +ALTER PUBLICATION testpub1_forschema DROP ALL TABLES IN SCHEMA pub_test1;
> +-- success
> +UPDATE pub_test1.tbl SET id = 2;
> +ALTER PUBLICATION testpub1_forschema ADD ALL TABLES IN SCHEMA pub_test1;
> +-- fail
> +UPDATE pub_test1.tbl SET id = 2;
> +ERROR: cannot update table "tbl" because it does not have a replica
> identity and publishes updates
> +HINT: To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
>
> I think here we don't need to test both SET and ADD variants, one of
> them is sufficient.
Removed it
> 7.
> +-- verify invalidation of partition table having partition on different
> schema
>
> I think this comment is not very clear to me. Can we change it to:
> "verify invalidation of partition table having parent and child tables
> in different schema"?
Modified
> 8.
> +-- verify invalidation of schema having partition parent table and
> partition child table
>
> Similarly, let's change this to: "verify invalidation of partition
> tables for schema publication that has parent and child tables of
> different partition hierarchies". Keep comments line boundary as 80
> chars, that way they look readable.
Modified
> 9.
> +++ b/src/test/subscription/t/025_rep_changes_for_schema.pl
> ..
> +# Basic logical replication test
>
> Let's change this comment to: "Logical replication tests for schema
> publications"
Modified
These comments are handled in the v38 patch attached at [2].
[1] -
https://www.postgresql.org/message-id/OS0PR01MB6113A715FB3B85907458F4E7FBB59%40OS0PR01MB6113.jpnprd01.prod.outlook.com
[2] -
https://www.postgresql.org/message-id/CALDaNm1TP9S0dif2QWoEUcCtNDop1xJ6Rj1xnu2vS92%3Dj9ahYw%40mail.gmail.com
Regards,
Vignesh
