Thread: Commitfest 2021-11 Patch Triage - Part 3
Welcome to the third installment of the CF 2021-11 patch triage, looking at the patches which have been in four CF's and parts of the ones with three CF's. As per usual I might very well have misunderstood something (or many things), but there seem to be a fair few ones which should be closeable in this CF here. 2962: Fix DROP TABLESPACE on Windows with ProcSignalBarrier? ============================================================ This fixes an old bug which can prevent DROP TABLESPACE from working on Windows. There has been a fair bit of review and testing, by among other myself, and I feel this patch is in good enough shape to be considered for Ready for Committer. There is a goalpost movement request in the thread for DROP DATABASE support, but I think we can get this in in two commits to move us forward. What are your thoughts on this one Thomas? 2979: Support tab completion for upper character inputs in psql =============================================================== There is broad concensus that this patch fixes a less than ideal behavior in psql, and the patch has seen a lot of review and rework. It currently needs a review of the latest version, but skimming it and judging by the thread I'd say it's close to Ready for Committer (if not already). 2942: increase size of pg_commit_ts buffers =========================================== Production use has validated this patch as fixing a performance regression, and the patch has support from another committer. There is mention of patch 2627 (More scalable multixacts buffers and locking) in the thread as an alternative approach, that buffers potentially should be configurable instead. Since that patch seems like further out from landing, should we go ahead with this to have at least a partial solution for 15? Do have any plans to go ahead with this patch Álvaro or are you awaiting results on the configurable approach? 2992: Allow batched insert during cross-partition updates ========================================================= This is a follow-up to 927f453a94106 to support batched inserts of internal INSERT commands which are used in cross-partition updates for local/remote partitions. The patch was after a lot of review initially marked Ready for Committer but went to WOA after failing to apply. Once updated it went to Need Review which IMO is misleading given it's status and a skim so I'm moving it to RFC again. Georgios, having reviewed it earlier do you any thoughts on the latest version? 2970: Set default transactions to read-only at servers start in pg_upgrade ========================================================================== Goalpost movement saw this patch go from read-only transactions to stopping bgworkers from running at all during pg_upgrade. There is agreement on the approach, but discussion on the exact implementation (which has since trailed off). I have on my TODO to bring this over the finish line. 2948: Simplify some RI checks to reduce SPI overhead ==================================================== Marked ready for committer it seems that raised concerns have been addressed. Peter, being listed as reviewer do you have any thoughts on this? 2957: Identify missing publications from publisher while create/alter subscription. ===================================================================== Patch is being actively reviewed, has been kept up to date, and was marked RFC back in May. Hopefully we can close this soon then. 2968: Minimal logical decoding on standbys (take 6) =================================================== The latest revision of the patch has had feedback from Robert (and Andres) which needs to be addressed, and the patch no longer applies (after the recent ThisTimeLineID changes perhaps?). I'm moving this patch to WoA for now. 2999: Catalog version access ============================ The initial patch in this thread was dismissed for not providing enough information to solve the stated problem, but there has since been a patch posted which performs ReadControlFile as recommended in the thread. Vik, can you confirm if this patch solves your original problem? 2996: Speed up verifying UTF-8 ============================== An IMO interesting approach to speeding up input validation with promising results posted. The discussion seem to be quite confident with the patch, but the latest version takes out a portion of the previous patch which was deemed overly complicated. Heikki, are you up for reviewing this version? 2932: Partial foreign key updates in referential integrity triggers =================================================================== A recently posted update from Peter indicates that this is close to being committable, so fingers crossed. 3025: postgres_fdw: suppress explicit casts in text:text comparisons ==================================================================== Currently undergoing active review and receiving updates with a new version posted today by Tom. Hopefully it can be closed in this CF. 2947: Full support for index LP_DEAD hint bits on standby ========================================================= This has potential to be a nice performance boost for a common usecase for scalable production setups. The patch changed course half-way through but has since been marked Ready for Committer. Any takers on this? 3120: Rewriting the test of pg_upgrade as a TAP test - take three ================================================================= Currently this is stuck on a discussion around how to deal with test.sh and the effects on the buildfarm. It seems like the questions raised have been answered, but we need to settle on how to continue. 3230: Column filtering in logical replication ============================================= This fails badly to apply, which is likely because there has been a lot of discussion on the grammar making a new patch not really an option until settled. That discussion ended in late September so hopefully there is a new version during this CF, else we'll have to close this with RwF awaiting a new patch in a later CF. 3223: Delegating superuser tasks to new security roles ====================================================== There is a lot of ongoing discussion on this one, but skimming the thread it's not really clear (to me) where it leaves the patch in question. 3138: Support for NSS as a libpq TLS backend ============================================ I'm clearly fairly biased here but I hope that at 48 revisions in the patch is in good enough shape to be close to RFC. Joshua Brindle has recently picked up reviewing it so hope we can reach somewhere for 15 with this work. 3035: Removing unused trailing linepointers from heap pages =========================================================== An alternative to this was committed for 14, which led the author to change the patch into a follow-up to that commit. The merit and value of this second patch is debated, with seemingly good arguments both for and against. There was an ask for testcases to provide evidence of correctness back in August which has gone unanswered, so unless this moves during this CF I think we should mark it RWF awaiting a new version of the patch. 3058: Eliminating "Permission denied" errors on stat() (on Windows) =================================================================== This was committed back in July, and then promptly reverted as there were issues with mingw. There has since been a patch posted addressing the problem which needs review. Michael, having looked at the earlier version, do you feel up for taking a look at the reworked one? 3172: Consistent use of SSL/TLS in docs ======================================= The proposal to use SSL/TLS instead of just SSL in the docs came about after a small discussion in the NSS thread, but it wasn't to anyones liking so the reworked patch just adds notes that SSL and TLS are used interchangeably in the docs. This version is a pretty small addition that I think we should go ahead with, but more eyes are needed to validate that. -- Daniel Gustafsson https://vmware.com/
> On Nov 11, 2021, at 2:12 PM, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> 3223: Delegating superuser tasks to new security roles
> ======================================================
> There is a lot of ongoing discussion on this one, but skimming the thread it's
> not really clear (to me) where it leaves the patch in question.
This set of patches got a fair amount of in-person off-list discussion at the
PGConf NYC with folks who had concerns about some or all parts of the patch.
Most of the known contentious design details were resolved, and I've been
updating the patches to repost this week. The subscriptions patch and guc
patch should be close to ready. The CREATEROLE patch may take a bit longer.
Conversation Highlights:
- The logical replication subscription patch needs to consider RLS.
(Jeff Davis)
This is implemented but I still need to update the documentation before
posting.
- The patch for granting SET VALUE and/or ALTER SYSTEM on guc settings should
be extended to allow revoking privilege to SET VALUE on userset gucs.
(Joshua Brindle, Stephen Frost).
This is implemented for core by means of converting PGC_USERSET variables
to PGC_SUSET, with a corresponding "GRANT SET VALUE ON .. TO public" added
to the new catalog/setting_privileges.sql file. By making it an explicit
grant, it is revokable without the need for any special logic. This would allow, for
example, REVOKE SET VALUE ON work_mem FROM public; followed by explicit
grants to whichever roles you want to allow to set work_mem.
I still need to adjust gucs defined in some contrib modules, update
documentation, and verify pg_upgrade from older versions before posting.
External projects are not harmed by this implementation. The only issue
for them is that SET VALUE cannot be revoked from any PGC_USERSET gucs they
define until they update their module to use PGC_SUSET plus a GRANT
somewhere in their extension creation or upgrade sql script.
- The CREATEROLE patch can go forward with roles having owners, but other
spec compliant behavior for GRANT/REVOKE should be implemented.
(Stephen Frost)
The bug wherein the grantor field in pg_auth_members may point to a dropped
role should be fixed. Probably this field should simply be dropped. We
don't use it anywhere, and on upgrade we can't know if existing values are
correct or bogus. Values that do not refer to any existing role are
obviously bogus, but if a role Oid was reassigned, a grantor entry may be
pointing wrongly at the new role, and there is no way to know that.
Stephen indicated that, for all privileges, only the role which GRANTed a
privilege should be able to REVOKE it. This differs from the current
implementation and is likely controversial. Robert Haas, at least,
described that idea as a non-starter. I don't plan to include that in this
patch, as it doesn't seem necessary to combine the two features even if we
eventually intend to do both. (And certainly unhelpful to include it if
the idea is rejected.)
Whether the owner of a role is implicitly a member (or admin) of the owned
role is still disputed. Stephen Frost would rather the owner has the right
to grant itself into the role, or grant itself ADMIN on the role, but not
be a member nor an admin by default. Robert Haas preferred that the owner
is both a member and an admin by default, making role ownership analogous
to how superuser works. I lean slightly in Stephen's favor, as any role
"foo" with CREATEROLE could always run:
CREATE ROLE bar [ ROLE foo | ADMIN foo];
and thereby give themselves the privilege they want in the same command
that creates the role. Going with Robert's proposal, a creator who does
not want to have privileges in the role would need to run:
CREATE ROLE bar;
REVOKE [ADMIN OPTION FOR ] bar FROM foo;
which requires two separate commands, and creates a short window inbetween
where a (possibly undesireable) configuration exists.
Jeff Davis and Andrew Dunstan have volunteered as committers.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, 2021-12-13 at 11:20 -0800, Mark Dilger wrote:
> - The logical replication subscription patch needs to consider RLS.
> (Jeff Davis)
>
> This is implemented but I still need to update the documentation
> before
> posting.
We also discussed how much of the insert path we want to include in the
apply worker. The immediate need is for the patch to support RLS, but
it raised the question: "why not just use the entire ExecInsert()
path?".
That's not a blocking issue for this patch, but something to consider
that will avoid problems if we want to support updatable views or
foreign tables as a target of a subscription.
Regards,
Jeff Davis
> On Dec 13, 2021, at 10:18 PM, Jeff Davis <pgsql@j-davis.com> wrote: > >> This is implemented but I still need to update the documentation >> before >> posting. > > We also discussed how much of the insert path we want to include in the > apply worker. The immediate need is for the patch to support RLS, but > it raised the question: "why not just use the entire ExecInsert() > path?". I went a different direction with this. The need is to prevent non-superuser subscription owners from *circumventing* RLS. For this patch set, I'm just checking whether RLS should be enforced against the subscription owner, and if so, raisingan ERROR, same as for a privilege violation. This should be sufficient in practice, as the table owner, roles withbypassrls privilege, and superusers should still be able to replicate into an RLS enabled table. The problem with actually *supporting* RLS is that the current logical replication implementation is a mix of different practical(rather than theoretical) design choices. After working to get RLS working as part of that, I became concernedthat there may be subtle differences between how I was making RLS work in logical replication vs. how it works forregular inserts/updates/deletes. Rather than create a mess that would need to be supported going forward, I bailed outand just forbade it. As far as completeness and a clean implementation (but not performance) are concerned, using ExecInsert is quite appealing. I think that would require reworking the logical replication protocol to send more than one row at a time, sothat the overhead of such a choice is not paid *per row*. That seems quite out of scope for this release cycle, though. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, 2021-12-14 at 07:41 -0800, Mark Dilger wrote:
> I went a different direction with this. The need is to prevent non-
> superuser subscription owners from *circumventing* RLS. For this
> patch set, I'm just checking whether RLS should be enforced against
> the subscription owner, and if so, raising an ERROR, same as for a
> privilege violation. This should be sufficient in practice, as the
> table owner, roles with bypassrls privilege, and superusers should
> still be able to replicate into an RLS enabled table.
I agree with this. Let's consider subscription targets involving RLS a
separate feature that is just blocked for now (except for bypassrls
roles and superusers).
> As far as completeness and a clean implementation (but not
> performance) are concerned, using ExecInsert is quite appealing. I
> think that would require reworking the logical replication protocol
> to send more than one row at a time, so that the overhead of such a
> choice is not paid *per row*. That seems quite out of scope for this
> release cycle, though.
Are you sure overhead is a problem? INSERT INTO ... SELECT goes through
that path. And the existing logical replication insert path does some
extra stuff, like opening the table for each row, so it's not clear to
me that logical replication is much faster.
Also, the current patch does per-row ACL checks. Did you consider
making the checks a part of logicalrep_rel_open()? That already has a
path to consider relcache invalidation, so it would also be a good
place to check if the table has RLS enabled.
Regards,
Jeff Davis