Date: Sat, 4 Jul 2026 07:46:30 -0700
From: Noah Misch <noah@leadboat.com>
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers@postgresql.org
Subject: Re: PROPERTY GRAPH pg_dump ACL minimization
Message-ID: <20260704144630.34.noahmisch@microsoft.com>
References: <20260630023308.c7.noahmisch@microsoft.com>
 <2453089.1782789061@sss.pgh.pa.us>
 <CAExHW5tz+ufmneS+gYTNrYLbi=bF=D2Xp5rvr12cuLYoNn80kg@mail.gmail.com>
 <CAExHW5va66cEd6uXvj16F0dGRfikWXf7jzRULrqV3OQ99EPsXg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAExHW5va66cEd6uXvj16F0dGRfikWXf7jzRULrqV3OQ99EPsXg@mail.gmail.com>
User-Agent: Mutt/2.3.0 (2026-01-25)
Archived-At: 
 <https://www.postgresql.org/message-id/20260704144630.34.noahmisch%40microsoft.com>
Precedence: bulk

On Wed, Jul 01, 2026 at 09:51:13PM +0530, Ashutosh Bapat wrote:
> On Tue, Jun 30, 2026 at 8:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Noah Misch <noah@leadboat.com> writes:
> > > Most of the patch bulk (modest as it is) exists to keep support for dumping
> > > from beta1.  I'm not sure whether it was worth bothering.  Breaking dump from
> > > a beta is without precedent known to me, so I just erred on the side of not
> > > breaking it.  If we were to decide pg_dump could drop support for betas, I'd
> > > be fine with that.
> >
> > > This entails a catversion bump on the v19 branch.
> >
> > Those points are not unrelated.  If you bump catversion then beta
> > testers must use pg_upgrade to get from beta1 to beta2, so you should
> > not drop support for dumping from beta1.
> >
> > I could agree with dropping that support after beta2, though.  That'd
> > imply having to update via beta2 to beta3 or later, but I doubt those
> > hardy enough to test beta1 would have a problem with that.

I agree dropping 19beta1 dump support after beta2 would be reasonable, and I
hadn't thought of doing so.  However, ...

> Should there be two commits one which will be reverted post beta2 and
> one which will stay after beta2?

... since the sole benefit would be removing ~25 lines, I don't think a
followup patch is worth it.  I wouldn't object if someone wants to remove that
code later.

> > > If the master branch
> > > already has a post-branch catversion bump by then, the two catversions should
> > > remain distinct.  I'll use yyyymmdd1 for v19 and yyyymmdd2 for master.
> >
> > Check.
> >
> > (I didn't read the patch, just responded to your commentary.)
> 
> I reviewed the patch.
> 
> - tblinfo[i].dacl.acldefault = pg_strdup(PQgetvalue(res, i, i_acldefault));
> + /* acldefault computed below */
> 
> Rather than spacially separating the acldefault computation, can we
> just write a function to compute the acldefault for a given relkind
> and owner, and call that function here?

Such a function would have just one caller, and that would even further
spatially separate the code lines defining the computation.  Either way is
fine, but I'm not inclined to change to that.

> Did you consider writing a test for the same?

Somewhat.  Long-term, I think a more general test would have a place.

> Other than that the patch looks good.

Thanks for reviewing.

> I wondered whether we are missing special handling for PROPGRAPH at
> other places. I looked at other places where we handle OBJECT_SEQUENCE
> separately in acl related files. I discovered following missing cases

This probably calls for its own thread; feel free to fork the thread for any
followup on that.

On Fri, Jul 03, 2026 at 04:09:20PM +0530, Ashutosh Bapat wrote:
> On Wed, Jul 1, 2026 at 9:51 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > I wondered whether we are missing special handling for PROPGRAPH at
> > other places. I looked at other places where we handle OBJECT_SEQUENCE
> > separately in acl related files. I discovered following missing cases
> >
> > 1. ExecGrant_Relation: I think we should clip the extra privileges
> > with a warning when GRANT ... TABLE syntax is used to grant privileges
> > on a property graph, just like sequences. To me it looks like we
> > should prohibit GRANT ... TABLE on property graph altogether. But
> > haven't done so to keep it in sync with sequences. The backward
> > compatibility comment,  "For backward compatibility, just ... " should
> > not be applicable in case of property graph since we can introduce
> > whatever behaviour we expect from GRANT ... TABLE right from the first
> > release which introduced property graph. But I am not sure if that's
> > the only backward compatibility we are talking about here. Those
> > commits go more than a few decades back and commit message itself
> > doesn't help me much. Maybe someone with a better historical
> > perspective may help. I have also added a test scenario for a
> > non-property graph privilege to be added using GRANT ... TABLE syntax.
> 
> Since property graphs share the namespace with regular tables, I think
> GRANT ... TABLE should be supported on property graphs, but restrict
> it to only the privileges applicable to property graphs.

I don't have a strong opinion on that, but I likely would have chosen to block
GRANT TABLE on a propgraph.  The backward compatibility argument written for
SEQUENCE likely means that someone noticed GRANT TABLE worked on sequences and
decided both that it was a mistake and that reversing the mistake would be a
cure worse than the disease.  There was a rebuttable presumption that when we
add a new grantable object with its own GRANT subtype, older GRANT subtypes
should reject that object.

> > 2. pg_class_aclmask_ext(): this seems to be another omission. Probably
> > innocuous since we will test only SELECT privileges on a property
> > graph and ignore other default table privileges.

Makes sense.