Re: Privileges on PUBLICATION

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:

> On 16.12.22 17:37, Antonin Houska wrote:
> > This is v4. The patch had to be rebased due to the commit 369f09e420.
>
> I think what this patch set needs first of all is a comprehensive description
> of what it is trying to do, exactly what commands and behaviors it adds, what
> are some of the subtleties and corner cases, what are open issues and
> questions. Some of that can be pieced together from this thread, but it
> should really be put in one place somewhere, ideally in the commit message
> and/or the documentation. (The main 0002 patch does not have any
> documentation.) It looks like you have a lot of bases covered, but without a
> full description, it's difficult to tell.
>
> Some points on the details:
>
> * You can combine all five patches into one. I don't think they are meant to
> be applied separately. The 0001 looks like it was maybe meant to be used
> separately, but it's not clear. Again, the overall description would help.
>
> * There is a lot of code that is contingent on am_db_walsender. We should
> avoid that. In most cases, it doesn't seem necessary. Or at least document
> the reasons.
>
> * The term "aware" (of a publication ACL, of a relation) is used a bunch of
> times. That's not a technical term, and the meaning of those phrases is not
> clear. Make sure the documentation/comments are precise.
>
> * I don't think using SPI is warranted here. You can get the required
> information directly from the underlying functions.
>
> * The places the privileges are ultimately checked is too unprincipled. The
> 0001 patch overrides a very low-level function, but the 0002 on the other
> hand checks the privileges by digging through the query structures by hand
> instead of letting the executor do it. We need to find ways to handle that
> that is more consistent with what the code is currently doing instead of
> adding more layers to it above and below.
>
> * The misc_sanity.out test output means you need to add a TOAST table to
> pg_publication.
>

Thanks for your review. Attached is a new version that tries to address your
findings.

I reworked the patch a bit, especially the handling of the PUBLICATION_NAMES
of the COPY TO command, so that compatibility with older subscribers is not
broken. The compatibility is actually the hardest part.

0001 only move some code into functions. I think it's better to do
this kind of thing separate so that the actual changes are easier to read.

The TODO comment in 0002 is related to [1].

[1] https://www.postgresql.org/message-id/3472.1675251957%40antos

--
Antonin Houska
Web: https://www.cybertec-postgresql.com