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.
