Re: Privileges on PUBLICATION

Peter Eisentraut <peter.eisentraut@enterprisedb.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