Feedback-ID: i131946ab:Fastmail
Message-ID: <485382af-1dff-b3b0-9ace-67bdd0082220@enterprisedb.com>
Date: Mon, 9 Jan 2023 10:23:32 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: Privileges on PUBLICATION
Content-Language: en-US
To: Antonin Houska <ah@cybertec.at>, pgsql-hackers@lists.postgresql.org
Cc: Euler Taveira <euler@eulerto.com>, Amit Kapila <amit.kapila16@gmail.com>
References: <20330.1652105397@antos>
 <8a00e0e1-dc4e-422c-9dcf-664b16b18625@www.fastmail.com>
 <CAA4eK1Lya6BoMa1m8cTZrQ8KvABQpn3EsHDKRurz8UJeHsGnZw@mail.gmail.com>
 <2810.1652171844@antos>
 <d96103fe-99e2-4119-bd76-952d326b7539@www.fastmail.com>
 <5859.1652423797@antos>
 <e6025f13-fefa-4ff2-9364-24f4c77fcf7a@www.fastmail.com>
 <57022.1652867097@antos>
 <4c420c17-8570-4b52-b7f6-9ce3f53d7ef9@www.fastmail.com>
 <14292.1655733695@antos.home>
 <746c9a65-e2cc-2573-a50e-ab5ea884f680@enterprisedb.com>
 <1537.1667454192@antos>
 <a7b95f74-e3e3-adae-b472-2e8d24a4e4b8@enterprisedb.com>
 <2497.1667546930@antos>
 <043c52a4-c001-0201-94fc-305dffb8313e@enterprisedb.com>
 <3862.1668424068@antos> <94545.1669732512@antos> <54334.1671208627@antos>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <54334.1671208627@antos>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/485382af-1dff-b3b0-9ace-67bdd0082220%40enterprisedb.com>
Precedence: bulk

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.