Feedback-ID: i131946ab:Fastmail
Message-ID: <b33b02c2-672c-5a02-5f4f-9a7d68910d48@enterprisedb.com>
Date: Mon, 20 Mar 2023 07:17:51 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.9.0
Subject: Re: Privileges on PUBLICATION
Content-Language: en-US
To: "Gregory Stark (as CFM)" <stark.cfm@gmail.com>,
 Antonin Houska <ah@cybertec.at>
Cc: pgsql-hackers@lists.postgresql.org, 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>
 <485382af-1dff-b3b0-9ace-67bdd0082220@enterprisedb.com>
 <6778.1675252927@antos>
 <CAM-w4HOqSnYidxmVodEkFD1S-7g2bPDbFK7zbaj4c12A1J5hAw@mail.gmail.com>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: 
 <CAM-w4HOqSnYidxmVodEkFD1S-7g2bPDbFK7zbaj4c12A1J5hAw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/b33b02c2-672c-5a02-5f4f-9a7d68910d48%40enterprisedb.com>
Precedence: bulk

On 14.03.23 19:30, Gregory Stark (as CFM) wrote:
> FYI this looks like it needs a rebase due to a conflict in copy.c and
> an offset in pgoutput.c.
> 
> Is there anything specific that still needs review or do you think
> you've handled all Peter's concerns? In particular, is there "a
> comprehensive description of what it is trying to do"? :)

The latest versions of the patch have pretty much addressed my initial 
comments.  The patch is structured and explained better now.  Most 
extraneous or incomplete changes have been addressed.

The problem now is that it's still a quite complicated patch that 
introduces a security feature.  It still touches a number of subsystems 
on different levels of abstraction.  This functionality is not of the 
kind, "if you don't use it it won't affect you", since it effectively 
pokes holes into the existing privileges checking in order to allow 
publication privileges checking to override it in some cases.  It will 
take significant effort to do a complete analysis and testing on whether 
it is secure and robust.  I don't think I will have time for that, and I 
don't think anyone will want to commit something like this at the last 
moment.

We have already taken a number of things from earlier patches and 
committed them separately as refactorings.  I don't see anything in the 
current patch anymore that we might want to take independently like that.

So in summary I think it would be best to keep this patch around for PG17.