Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1ulNqj-00ELz0-9W for pgsql-hackers@arkaria.postgresql.org; Mon, 11 Aug 2025 08:25:45 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1ulNqh-00B0pp-Ut for pgsql-hackers@arkaria.postgresql.org; Mon, 11 Aug 2025 08:25:43 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1ulNqh-00B0ph-Fl for pgsql-hackers@lists.postgresql.org; Mon, 11 Aug 2025 08:25:43 +0000 Received: from mail-qt1-x830.google.com ([2607:f8b0:4864:20::830]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1ulNqf-001qir-03 for pgsql-hackers@lists.postgresql.org; Mon, 11 Aug 2025 08:25:42 +0000 Received: by mail-qt1-x830.google.com with SMTP id d75a77b69052e-4b0739c6557so62301931cf.3 for ; Mon, 11 Aug 2025 01:25:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1754900740; x=1755505540; darn=lists.postgresql.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=SclMLkKLYCs7SMCWWVAfXvqA/g+8cuJnY3a8ec1zeBI=; b=DrHPSmSelD7quJf/blJ7OM59Pu1varGeN3iscOWRGTeU6ceGyuKSXVpwOsHFi4oK4Q +pV7cAYQpt9k0/4cg0gqYdCNrTZ2CuIUeq1qWzicQI89cpKpTDxedJvmPpdAvZduTTfh bG+qH2LBcmLKunJYk48XL/cuon+3PceVh7ZeuTNYsMdEpn7xV5RvxuspqV1nGznQzuMC omiIpgrCoc6ChI8Z7BlDgXYdBZ5bFwedAuV26iVdSU0iapT4Ha7HNNhJmLmUebB/1LLM oja++7JrqjctE0NyQwYgocFen8a55ijdAf6YRm+xFCkPrqEU++p+T0IKzkCvsJcSivlt o2Pw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1754900740; x=1755505540; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=SclMLkKLYCs7SMCWWVAfXvqA/g+8cuJnY3a8ec1zeBI=; b=YKKDlZ0hqFbSaC2czdPeMxcJynxfg4UnGXvQLxwfPH4iqA4sgPSYkUsgaDyAtJghPF wXPAFym/cUVFqsJaYJsbwBS+XWWmocj3fOB5ze1WWHTkR3PLn09dFtzRhb9W1YAa2pEZ TnwmHIcRduJLiIEzI2o6DVwAONBffPzsg3SUhrh+uZV5ULiElGCNsuNOHDMFGZmDokmb P59q/YkkB/WSMyHe5TzQ0iqNXZAJ+gjlyhAo28OjhhkJ4/5Md7ssV6fqhco4hqvz6wCu ivWHsjgOX9v6GQmO3vEMpurDbIF0WjXGqTwrICcqKxXI9/wehpiS3nREDnLbwgg3F7lb H9eg== X-Forwarded-Encrypted: i=1; AJvYcCVE77T9JeTbhhO6AvoqEP+k0Ojoyn1/bdq041+QjfEc3MzFwCqcnToJg8yhwkKSlxM1DcNfmSPPJin4tOoH@lists.postgresql.org X-Gm-Message-State: AOJu0YxPuVOhdZUOQRWedqZ9+quDdzdI4P3EVDao48tOjvhAcqMaqKv8 2MX4E3K+roqfxr3fR+o4WnENlG11SKVTs61XqYpEbe/JTAkyezL4gbRf1tEhVzixaPFVLArOAH/ c5oktA0s4rghqa/wZf7HW9jM5DRWR/lo= X-Gm-Gg: ASbGnctlGDZ1l3mP1LoyV5cuvf1znWuL5NaSxgnskuD9HthQY+2PSWqmmrOzsHmEkFZ O+o0y6mO3Lvg+h3kAmtq1XInib/YNKa3zMn8L3sXlVP8GajNRAdLeiZa26XLDAlRpuuX4Hch6aR hJjXcYN/ebUPjiCC+G0E5ZKyL2yKGu+6KsO+j35JeK79Nelp6lnW/vZLboLEUtUii+EHzHC5GK7 7LSBCA6is6dvuAdmCZsmYt6Ig0B82BQgNpUlwwzHQ== X-Google-Smtp-Source: AGHT+IHAlWv+b0sqvq1flNUqPN0BuDMHlLC8+4s0259MKCDPFOq5TvhFO7R4Va0EZWZiMJmz6ywNMsegOcHBUGLwhRA= X-Received: by 2002:ac8:5d15:0:b0:4b0:822e:9a3b with SMTP id d75a77b69052e-4b0aedd7c9dmr158472591cf.28.1754900739817; Mon, 11 Aug 2025 01:25:39 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Mon, 11 Aug 2025 18:25:12 +1000 X-Gm-Features: Ac12FXzdhiTHjduqbFzE0i1wmfiL1f9B9eCmZ1ZSbvGqzkMwxh_LNiaqMgLT9pI Message-ID: Subject: Re: Skipping schema changes in publication To: Shlok Kyal Cc: Amit Kapila , "Zhijie Hou (Fujitsu)" , vignesh C , YeXiu <1518981153@qq.com>, Ian Lawrence Barwick , Bharath Rupireddy , PostgreSQL Hackers Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk Hi Shlok. On Wed, Aug 6, 2025 at 11:11=E2=80=AFPM Shlok Kyal wrote: > ... > > 5. > > Bitmapset *cols =3D NULL; > > + bool except_columns =3D false; > > + bool no_col_published =3D false; > > > > There are multiple places in this patch that say: > > > > 'no_col_published' > > or 'no_cols_published' > > > > I felt this var name can be misunderstood because it is easy to read > > "no" as meaning "no." (aka number), and then misinterpret as > > "number_of_cols_published". > > > > Maybe an unambiguous name can be found, like > > - 'zero_cols_published' or > > - 'nothing_published' or > > - really make it 'num_cols_published' and check for 0. > > > > (so this comment applies to multiple places in the patch) > > > How about 'all_cols_excluded'? Or 'has_published_cols'? > I have used 'all_cols_excluded' in this patch. Thoughts? The new name is good. > > =3D=3D=3D=3D=3D=3D > > src/bin/psql/describe.c > > > > describeOneTableDetails: > > > > 7. > > /* column list (if any) */ > > if (!PQgetisnull(result, i, 2)) > > - appendPQExpBuffer(&buf, " (%s)", > > - PQgetvalue(result, i, 2)); > > + { > > + if (strcmp(PQgetvalue(result, i, 3), "t") =3D=3D 0) > > + appendPQExpBuffer(&buf, " EXCEPT (%s)", > > + PQgetvalue(result, i, 2)); > > + else > > + appendPQExpBuffer(&buf, " (%s)", > > + PQgetvalue(result, i, 2)); > > + } > > > > Isn't this code fragment (and also surrounding code) using the same > > logic as what is already encapsulated in the function > > addFooterToPublicationDesc()? > > Superficially, it seems like a large chunk can all be replaced with a > > single call to the existing function. > > > 'addFooterToPublicationDesc' is called when we use \dRp+ and print in for= mat: > "schema_name.table_name" EXCEPT (column-list) > Whereas code pasted above is executed when we use \d+ table_name and > the output is the format: > "publication_name" EXCEPT (column-list) > > These pieces of code are used to print different info. One is used to > print info related to tables and the other is used to print info > related to publication. > Should we use a common function for this? It still seems like quite a lot of overlap. e.g. I thought there were ~30 lines common. OTOH, perhaps you'll need to pass another boolean to the function to indicate it is a "Publication:" footer. I guess you'd have to try it out first to see if the changes required to save those 30 LOC are worthwhile or not. > > > =3D=3D=3D=3D=3D=3D > > src/test/regress/expected/publication.out > > > > 8. > > +-- Syntax error EXCEPT without a col-list > > +CREATE PUBLICATION testpub_except2 FOR TABLE pub_test_except1 EXCEPT; > > +ERROR: EXCEPT clause not allowed for table without column list > > +LINE 1: CREATE PUBLICATION testpub_except2 FOR TABLE pub_test_except..= . > > + ^ > > > > Is that a bad syntax position marker (^)? e.g. Why is it pointed at > > the word "TABLE" instead of "EXCEPT"? > > > In function 'preprocess_pubobj_list' the position of position marker > (^) is decided by "pubobj->location". Function handles multiple errors > and setting "$$->location" only specific to EXCEPT qualifier would not > be appropriate. One solution I feel is to not show "position marker > (^)" in the case of EXCEPT. Or maybe we can add a new variable to > 'PublicationTable' for except_location but I think we should not do > that. Thoughts? In the review comments below, I suggest putting this location back, but changing the message. > > For this version of patch, I have removed the "position marker (^)" in > the case of EXCEPT. > ////// Here are my review comments for the patch v19-0003. =3D=3D=3D=3D=3D=3D 1. General - SGML tags in docs for table/column names. There is nothing to change just yet, but keep an eye on the thread [1], because if/when that gets pushed, then there will several tags in this patch for table/column names that will need to be updated for consistency. =3D=3D=3D=3D=3D=3D src/backend/catalog/pg_publication.c pg_get_publication_tables: 2. + + if (!nulls[2]) + { + Datum exceptDatum; + bool isnull; + + /* + * We fetch pubtuple if publication is not FOR ALL TABLES and + * not FOR TABLES IN SCHEMA. So if prexcept is true, it + * indicates that prattrs contains columns to be excluded for + * replication. + */ + exceptDatum =3D SysCacheGetAttr(PUBLICATIONRELMAP, pubtuple, + Anum_pg_publication_rel_prexcept, + &isnull); + + if (!isnull && DatumGetBool(exceptDatum)) + except_columns =3D pub_collist_to_bitmapset(NULL, values[2], NULL); + } Maybe this should be done a few lines earlier, to keep all the values[2]/nulls[2] code together, ahead of the values[3]/nulls[3] code. Indeed, there is lots of other values[2]/nulls[2] logic that comes later in this function, so maybe it is better to do all of that first, instead of mingling it with values[3]/nulls[3]. =3D=3D=3D=3D=3D=3D src/backend/commands/publicationcmds.c pub_contains_invalid_column: 3. * 1. Ensures that all columns referenced in the REPLICA IDENTITY are cove= red - * by the column list. If any column is missing, *invalid_column_list i= s set + * by the column list and are not part of column list specified with EX= CEPT. + * If any column is missing, *invalid_column_list is set * to true. Whitespace problem here; there is some tab instead of space in this comment= . Also /part of column list/part of the column list/ ~~~ AlterPublicationTables: 4. bool isnull =3D true; Datum whereClauseDatum; Datum columnListDatum; + Datum exceptDatum; It's not necessary to have all these different Datum variables; they are only temporary storage. It might be simpler to use a single "Datum datum;" which is reused 3x. ~ 5. + exceptDatum =3D SysCacheGetAttr(PUBLICATIONRELMAP, rftuple, + Anum_pg_publication_rel_prexcept, + &isnull); + + if (!isnull) + oldexcept =3D DatumGetBool(exceptDatum); + Isn't the 'prexcept' also used for EXCEPT TABLE as well as EXCEPT (column-list)? In other words, should the change to this function be done already in one of the earlier patches? ~ 6. if (equal(oldrelwhereclause, newpubrel->whereClause) && - bms_equal(oldcolumns, newcolumns)) + bms_equal(oldcolumns, newcolumns) && + oldexcept =3D=3D newpubrel->except) The code comment about this code fragment should also mention EXCEPT. =3D=3D=3D=3D=3D=3D src/backend/parser/gram.y preprocess_pubobj_list: 7. + if (pubobj->pubtable && pubobj->pubtable->except && + pubobj->pubtable->columns =3D=3D NULL) + ereport(ERROR, + errcode(ERRCODE_SYNTAX_ERROR), + errmsg("EXCEPT clause not allowed for table without column list")); + Having the syntax error location (like before in v18) might be better, but since that location is associated with the TABLE, then the error message should also be reworded so the subject is the table. SUGGESTION errmsg("table without column list cannot use EXCEPT clause") =3D=3D=3D=3D=3D=3D src/bin/psql/describe.c describeOneTableDetails: 8. - if (pset.sversion >=3D 150000) + if (pset.sversion >=3D 190000) { printfPQExpBuffer(&buf, "SELECT pubname\n" " , NULL\n" " , NULL\n" + " , NULL\n" "FROM pg_catalog.pg_publication p\n" " JOIN pg_catalog.pg_publication_namespace pn ON p.oid =3D pn.pnpub= id\n" " JOIN pg_catalog.pg_class pc ON pc.relnamespace =3D pn.pnnspid\n" @@ -3038,35 +3039,62 @@ describeOneTableDetails(const char *schemaname, " pg_catalog.pg_attribute\n" " WHERE attrelid =3D pr.prrelid AND attnum =3D prattrs[s])\n" " ELSE NULL END) " + " , prexcept " "FROM pg_catalog.pg_publication p\n" " JOIN pg_catalog.pg_publication_rel pr ON p.oid =3D pr.prpubid\n" " JOIN pg_catalog.pg_class c ON c.oid =3D pr.prrelid\n" - "WHERE pr.prrelid =3D '%s'\n", - oid, oid, oid); - - if (pset.sversion >=3D 190000) - appendPQExpBufferStr(&buf, " AND NOT pr.prexcept\n"); + "WHERE pr.prrelid =3D '%s' " + "AND c.relnamespace NOT IN (\n " + " SELECT pnnspid FROM\n" + " pg_catalog.pg_publication_namespace)\n" - appendPQExpBuffer(&buf, "UNION\n" "SELECT pubname\n" " , NULL\n" " , NULL\n" + " , NULL\n" "FROM pg_catalog.pg_publication p\n" - "WHERE p.puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n= ", - oid); - - if (pset.sversion >=3D 190000) - appendPQExpBuffer(&buf, - " AND NOT EXISTS (\n" - " SELECT 1\n" - " FROM pg_catalog.pg_publication_rel pr\n" - " JOIN pg_catalog.pg_class pc\n" - " ON pr.prrelid =3D pc.oid\n" - " WHERE pr.prrelid =3D '%s' AND pr.prpubid =3D p.oid)\n", - oid); - - appendPQExpBufferStr(&buf, "ORDER BY 1;"); + "WHERE p.puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n= " + " AND NOT EXISTS (\n" + " SELECT 1\n" + " FROM pg_catalog.pg_publication_rel pr\n" + " JOIN pg_catalog.pg_class pc\n" + " ON pr.prrelid =3D pc.oid\n" + " WHERE pr.prrelid =3D '%s' AND pr.prpubid =3D p.oid)\n" + "ORDER BY 1;", + oid, oid, oid, oid, oid); + } + else if (pset.sversion >=3D 150000) + { + printfPQExpBuffer(&buf, + "SELECT pubname\n" + " , NULL\n" + " , NULL\n" + "FROM pg_catalog.pg_publication p\n" + " JOIN pg_catalog.pg_publication_namespace pn ON p.oid =3D pn.pnpub= id\n" + " JOIN pg_catalog.pg_class pc ON pc.relnamespace =3D pn.pnnspid\n" + "WHERE pc.oid =3D'%s' and pg_catalog.pg_relation_is_publishable('%s')\n= " + "UNION\n" + "SELECT pubname\n" + " , pg_get_expr(pr.prqual, c.oid)\n" + " , (CASE WHEN pr.prattrs IS NOT NULL THEN\n" + " (SELECT string_agg(attname, ', ')\n" + " FROM pg_catalog.generate_series(0, pg_catalog.array_upper(pr.prattrs::pg_catalog.int2[], 1)) s,\n" + " pg_catalog.pg_attribute\n" + " WHERE attrelid =3D pr.prrelid AND attnum =3D prattrs[s])\n" + " ELSE NULL END) " + "FROM pg_catalog.pg_publication p\n" + " JOIN pg_catalog.pg_publication_rel pr ON p.oid =3D pr.prpubid\n" + " JOIN pg_catalog.pg_class c ON c.oid =3D pr.prrelid\n" + "WHERE pr.prrelid =3D '%s'\n" + "UNION\n" + "SELECT pubname\n" + " , NULL\n" + " , NULL\n" + "FROM pg_catalog.pg_publication p\n" + "WHERE p.puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n= " + "ORDER BY 1;", + oid, oid, oid, oid); I found these large SQL selects with 3x UNIONs are difficult to read. Maybe you can add more comments to describe the intention of each of the UNION SELECTs? ~~~ 9. /* column list (if any) */ if (!PQgetisnull(result, i, 2)) - appendPQExpBuffer(&buf, " (%s)", - PQgetvalue(result, i, 2)); + { + if (strcmp(PQgetvalue(result, i, 3), "t") =3D=3D 0) + appendPQExpBuffer(&buf, " EXCEPT"); + appendPQExpBuffer(&buf, " (%s)", PQgetvalue(result, i, 2)); + } I did not find any regression test case where the "EXCEPT" col-list is getting output for a "Publications:" footer. =3D=3D=3D=3D=3D=3D [1] https://www.postgresql.org/message-id/aIELRMAviNiUL1ie%40momjian.us Kind Regards, Peter Smith. Fujitsu Australia