On Mon, 14 Apr 2025 at 08:26, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh,
>
> Some review comments for patch v20250325-0002
>
> ======
> Commit message
>
> 1.
> Furthermore, enhancements to psql commands (\d and \dRp) now allow for better
> display of publications containing specific sequences or sequences included
> in a publication.
>
> ~
>
> That doesn't seem as clear as it might be. Also, IIUC the "sequences
> included in a publication" is not actually implemented yet -- there is
> only the "all sequences" flag.
>
> SUGGESTION
> Furthermore, enhancements to psql commands now display which
> publications contain the specified sequence (\d command), and if a
> specified publication includes all sequences (\dRp command)
Modified
> ======
> doc/src/sgml/ref/create_publication.sgml
>
> 2.
> <para>
> To add a table to a publication, the invoking user must have ownership
> - rights on the table. The <command>FOR ALL TABLES</command> and
> - <command>FOR TABLES IN SCHEMA</command> clauses require the invoking
> + rights on the table. The <command>FOR TABLES IN SCHEMA</command>,
> + <command>FOR ALL TABLES</command> and
> + <command>FOR ALL SEQUENCES</command> clauses require the invoking
> user to be a superuser.
>
> IMO these should all be using <literal> SGML markup same as elsewhere
> on this page, not <command> markup.
Modified
> ======
> src/backend/commands/publicationcmds.c
>
> 3.
> if (!superuser_arg(newOwnerId))
> {
> if (form->puballtables)
> ereport(ERROR,
> errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("permission denied to change owner of publication \"%s\"",
> NameStr(form->pubname)),
> errhint("The owner of a FOR ALL TABLES publication must be
> a superuser."));
> if (form->puballsequences)
> ereport(ERROR,
> errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("permission denied to change owner of publication \"%s\"",
> NameStr(form->pubname)),
> errhint("The owner of a FOR ALL SEQUENCES publication must
> be a superuser."));
> if (is_schema_publication(form->oid))
> ereport(ERROR,
> errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("permission denied to change owner of publication \"%s\"",
> NameStr(form->pubname)),
> errhint("The owner of a FOR TABLES IN SCHEMA publication
> must be a superuser."));
> }
>
> I wondered if there's too much duplicated code here. Maybe it's better
> to share a common ereport?
>
> SUGGESTION
>
> if (!superuser_arg(newOwnerId))
> {
> char *hint_msg = NULL;
>
> if (form->puballtables)
> hint_msg = _("The owner of a FOR ALL TABLES publication must be a
> superuser.");
> else if (form->puballsequences)
> hint_msg = _("The owner of a FOR ALL SEQUENCES publication must be
> a superuser.");
> else if (is_schema_publication(form->oid))
> hint_msg = _("The owner of a FOR TABLES IN SCHEMA publication must
> be a superuser.");
> if (hint_msg)
> ereport(ERROR,
> errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("permission denied to change owner of publication \"%s\"",
> NameStr(form->pubname)),
> errhint(hint_msg));
> }
I felt the existing code is ok in this case. It will be easier to
review if the error hint is along with ereport in this case.
> ======
> src/bin/psql/describe.c
>
> describeOneTableDetails:
>
> 4.
> + res = PSQLexec(buf.data);
> + if (!res)
> + goto error_return;
> +
> + numrows = PQntuples(res);
> +
>
> Isn't this same code already done a few lines above in the same
> function? Maybe I misread something.
Modified
> ======
> src/test/regress/sql/publication.sql
>
> 5.
> +-- check that describe sequence lists all publications the sequence belongs to
>
> Might be clearer to say: "lists both" instead of "lists all"
Modified
Regarding comment at [1]. On further thinking I have removed that test
as one test is enough for that change, so the comment handling is no
more required.
Regarding comment at [2]. The attached patch has the rebased changes too.
The attached v20250414 version patch has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHut%2BPsgZkEegDzhJ2%3DDwDkrks6g6aQ6LX1-M%2BXBBt4PP-MX3g%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAHut%2BPv5XMnX%2BQSSDhL5eqXV%3Dkp22jyYOgFx_u7kSMhwvktvrg%40mail.gmail.com
Regards,
Vignesh
