Re: pgsql: Doc: Explain about Column List feature.

On Tue, Sep 13, 2022 at 10:11 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2022-Sep-07, Amit Kapila wrote:
>
> > Doc: Explain about Column List feature.
> >
> > Add a new logical replication section for "Column Lists" (analogous to the
> > Row Filters page). This explains how the feature can be used and the
> > caveats in it.
> >
> > Author: Peter Smith
> > Reviewed-by: Shi yu, Vignesh C, Erik Rijkers, Amit Kapila
> > Backpatch-through: 15, where it was introduced
> > Discussion: https://postgr.es/m/CAHut+PvOuc9=_4TbASc5=VUqh16UWtFO3GzcKQK_5m1hrW3vqg@mail.gmail.com
>
> Hi
>
> I just read these docs and noticed that it mentions that column lists
> can be used for security. As far as I remember, this is wrong: it is
> the subscriber that builds the query to read column data during initial
> sync, and the publisher doesn't forbid to read columns not in it, so it
> is entirely possible for a malicious subscriber to read columns other
> than those published. I'm pretty sure we discussed this at some point
> during development of the feature.
>
> So I suggest to mention this point explicitly in its own paragraph, to
> avoid giving a false sense of security.
>

Thanks for the feedback.

The mention of 'security' in the page (as previously written) just
means to say that publications can prevent sensitive columns from
being replicated/subscribed by default. It was not intended to imply
those columns are immune from a malicious attack. Indeed, just having
another publication without any column lists could expose the same
sensitive columns.

I am fine with your rewording of the security part.

> While going over this text I found some additional things that could
> --in my opinion-- stand some improvement:

In general (because they have a lot of similarities), the wording and
the section structure of the "Column Lists" page tried to be
consistent with the "Row Filters" page. Perhaps this made everything
unnecessarily complex. Anyway, your suggested re-wording and removal
of those sections look OK to me - the content is the same AFAICT.

>
> * It feels better to start the section saying that a list can be
> specified; subscriber must have all those columns; omitting list
> means to publish everything. That leads to shorter text (no need to
> say "you need to have them all, oh wait you might only have a few").
>
> * there's no reason to explain the syntax in vague terms and refer the
> reader to the reference page.
>
> * The first few <sect2> seem to give no useful structure, and instead
> cause the text to become disorganized. I propose to remove them, and
> instead mix the paragraphs in them so that we explain the rules and
> the behavior, and lastly the effect on specific commands.
>
> The attached patch effects those changes.
>

For some reason I was unable to apply your supplied patch to master:

[postgres(at)CentOS7-x64 oss_postgres_misc]$ git apply column-list-wording.patch
column-list-wording.patch:16: trailing whitespace.
Each publication can optionally specify which columns of each table are
column-list-wording.patch:17: trailing whitespace.
replicated to subscribers. The table on the subscriber side must have at
column-list-wording.patch:18: trailing whitespace.
least all the columns that are published. If no column list is specified,
column-list-wording.patch:19: trailing whitespace.
then all columns in the publisher are replicated.
column-list-wording.patch:20: trailing whitespace.
See <xref linkend="sql-createpublication"/> for details on the syntax.
error: patch failed: doc/src/sgml/logical-replication.sgml:1093
error: doc/src/sgml/logical-replication.sgml: patch does not apply
error: patch failed: doc/src/sgml/ref/create_publication.sgml:94
error: doc/src/sgml/ref/create_publication.sgml: patch does not apply

>
> One more thing. There's a sect2 about combining column list. Part of it
> seems pretty judgmental and I see no reason to have it in there; I
> propose to remove it (it's not in this patch). I think we should just
> say it doesn't work at present, here's how to work around it, and
> perhaps even say that we may lift the restriction in the future. The
> paragraph that starts with "Background:" is IMO out of place, and it
> repeats the mistake that column lists are for security.
>

It wasn't clear which part you felt was judgemental. I have removed
the "Background" paragraph but I have otherwise left that section and
Warning as-is because it still seemed useful for the user to know. You
can change/remove it if you disagree.

>
> Lastly: In the create-publication reference page I think it would be
> better to reword the Parameters section a bit. It mentions
> FOR TABLE as a parameter, but the parameter is actually
> <replaceable>table_name</replaceable>; and the row-filter and
> column-list explanations are also put in there when they should be in
> their own <replaceable>expression</> and <replaceable>column_name</>
> varlistentries. I think splitting things that way would result in a
> clearer explanation.
>

IMO this should be proposed as a separate patch. Some of those things
(e.g. FOR TABLE as a parameter) seem to have been written that way
since PG10.

~~~

PSA a new patch for the "Column Lists" page. AFAIK this is the same as
everything that you suggested (except for the Warning section which
was kept as mentioned above).

------
Kind Regards,
Peter Smith.
Fujitsu Australia