On Mon, Jul 8, 2024, at 10:34, Michael Paquier wrote:
> Thanks for the patch. I have been looking at it for a few hours,
> eyeing a bit on the ObjectProperty parts a bit if we were to extend it
> for sub-object IDs, and did not like the complexity this introduces,
> so I'd be OK to live with the extra handling in pg_get_acl() itself.
>
> + /* ignore dropped columns */
> + if (atttup->attisdropped)
> + {
> + ReleaseSysCache(tup);
> + PG_RETURN_NULL();
> + }
>
> Hmm. This is an important bit and did not consider it first. That
> makes the use of ObjectProperty less flexible because we want to look
> at the data in the pg_attribute tuple to be able to return NULL in
> this case.
>
> I've tweaked a bit what you are proposing, simplifying the code and
> removing the first batch of queries in the tests as these were less
> interesting. How does that look?
Thanks, nice simplifications.
I agree the tests you removed are not that interesting.
Looks good to me.
Patch didn't apply to HEAD nor on top of any of the previous commits either,
but I couldn't figure out why based on the .rej files, strange.
I copy/pasted the parts from the patch to test it. Let me know if you need a
rebased version of it, in case you will need to do the same, to save some work.
Also noted the below in your last commit:
a6417078c414 has introduced as project policy that new features
committed during the development cycle should use new OIDs in the
[8000,9999] range.
4564f1cebd43 did not respect that rule, so let's renumber pg_get_acl()
to use an OID in the correct range.
Thanks for the info!
Will make sure to use the `src/include/catalog/renumber_oids.pl` tool
for future patches.
Regards,
Joel
