Re: Privileges on PUBLICATION

From: Antonin Houska <ah(at)cybertec(dot)at>
To: Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org, Euler Taveira <euler(at)eulerto(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Subject: Re: Privileges on PUBLICATION
Date: 2023-02-01 12:02:07
Message-ID: 6778.1675252927@antos
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

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].


Antonin Houska

Attachment Content-Type Size
v05-0001-Move-some-code-into-functions.patch text/x-diff 20.6 KB
unknown_filename text/plain 122.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Melih Mutlu 2023-02-01 12:07:25 Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
Previous Message Alvaro Herrera 2023-02-01 12:01:26 Re: Progress report of CREATE INDEX for nested partitioned tables