On Mon, Apr 11, 2022 at 7:06 AM Justin Pryzby <pry...@telsasoft.com> wrote:
> On Sat, Apr 02, 2022 at 07:21:11PM +0200, Alvaro Herrera wrote: > > Small things here. > > > 1. in VACUUM FULL we only process partitions that are owned by the > > invoking user. We don't have this test in the new code. I'm not sure > > why do we do that there; is it worth doing the same here? > > That dates to a556549d7 (see also cbe24a6dd8 for an earlier commit in > CLUSTER > itself). The reason was to avoid blocking if an unprivileged user runs > VACUUM > FULL which would try to lock things (including shared catalogs) before > checking > if they have permission to vacuum them. That commit also initially checks > the > owner of the partitioned table, and then re-checking owner of partitions > later > on. > > A similar issue exists here. But 1) catalog tables are not partitioned, > and, > 2) ownership of a partitioned table is checked immediately. So the > problem can > only occur if a user who owns a partitioned table but doesn't own all its > partitions tries to cluster it, and it blocks behind another session. > Fixing > this is probably a good idea, but seems improbable that it would avoid a > DOS. > > > 2. We should silently skip a partition that's a foreign table, I > > suppose. > > I think it's not needed, since the loop over index children doesn't see a > child > index on the foreign table. ? > > > 3. We do mark the index on the partitions as indisclustered AFAICS (we > > claim that the partitioned table's index is not marked, which is > > accurate). So users doing unadorned CLUSTER afterwards will get the > > partitions clustered too, once they cluster the partitioned table. If > > they don't want this, they would have to ALTER TABLE to remove the > > marking. How likely is that this will be a problem? Maybe documenting > > this point is enough. > > It seems at least as likely that someone would *want* the partitions to be > marked clustered as that someone would want them to be unchanged. > > The cluster mark accurately reflects having been clustered. It seems > unlikely > that a user would want something else to be clustered later by "cluster;". > Since clustering on a partitioned table wasn't supported before, nothing > weird > will happen to someone who upgrades to v15 unless they elect to use the new > feature. As this seems to be POLA, it doesn't even need to be > documented. ? > Hi, For v13-0002-cluster-early-ownership-check-of-partitions.patch : only for it to fails ownership check anyway to fails -> to fail Cheers
