Обсуждение: Collect ObjectAddress for ATTACH DETACH PARTITION to use in event trigger

Hi hackers,

I noticed that we didn't collect the ObjectAddress returned by
ATExec[Attach|Detach]Partition. I think collecting this information can make it
easier for users to get the partition OID of the attached or detached table in
the event trigger. So how about collecting it like the attached patch ?

Best regards,
Hou zhijie

On Wednesday, July 13, 2022 5:58 PM Hou, Zhijie wrote:
> Hi hackers,
>
> I noticed that we didn't collect the ObjectAddress returned by
> ATExec[Attach|Detach]Partition. I think collecting this information can make it
> easier for users to get the partition OID of the attached or detached table in
> the event trigger. So how about collecting it like the attached patch ?

Added to next CF.

Best regards,
Hou zhijie

Hi,

> > I noticed that we didn't collect the ObjectAddress returned by
> > ATExec[Attach|Detach]Partition. I think collecting this information can make it
> > easier for users to get the partition OID of the attached or detached table in
> > the event trigger. So how about collecting it like the attached patch ?
>
> Added to next CF.

Sounds good. I grepped ATExecXXX() functions called in ATExecCmd(),
and I confirmed that all returned values have been collected except them.

While checking test code test about EVENT TRIGGER,
I found there were no tests related with partitions in that.
How about adding them?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Jul 15, 2022 at 03:21:30AM +0000, kuroda.hayato@fujitsu.com wrote:
> Sounds good. I grepped ATExecXXX() functions called in ATExecCmd(),
> and I confirmed that all returned values have been collected except them.
>
> While checking test code test about EVENT TRIGGER,
> I found there were no tests related with partitions in that.
> How about adding them?

Agreed.  It would be good to track down what changes once those
ObjectAddresses are collected.
--
Michael

On Friday, July 15, 2022 11:41 AM Michael Paquier <michael@paquier.xyz> wrote:

Hi,
>
> On Fri, Jul 15, 2022 at 03:21:30AM +0000, kuroda.hayato@fujitsu.com wrote:
> > Sounds good. I grepped ATExecXXX() functions called in ATExecCmd(),
> > and I confirmed that all returned values have been collected except them.
> >
> > While checking test code test about EVENT TRIGGER, I found there were
> > no tests related with partitions in that.
> > How about adding them?
>
> Agreed.  It would be good to track down what changes once those
> ObjectAddresses are collected.

Thanks for having a look. It was a bit difficult to add a test for this.
Because we currently don't have a user function which can return these
collected ObjectAddresses for ALTER TABLE. And It seems we don't have tests for
already collected ObjectAddresses as well :(

The collected ObjectAddresses is in
"currentEventTriggerState->currentCommand->d.alterTable.subcmds.address" while
the public function pg_event_trigger_ddl_commands doesn't return these
information. It can only be used in user defined event trigger function (C
code).

If we want to add some tests for both already existed and newly added
ObjectAddresses, we might need to add some test codes in test_ddl_deparse.c.
What do you think ?

Best regards,
Hou zhijie

Dear Hou-san,

> Thanks for having a look. It was a bit difficult to add a test for this.
> Because we currently don't have a user function which can return these
> collected ObjectAddresses for ALTER TABLE. And It seems we don't have tests for
> already collected ObjectAddresses as well :(
> The collected ObjectAddresses is in
> "currentEventTriggerState->currentCommand->d.alterTable.subcmds.address" while
> the public function pg_event_trigger_ddl_commands doesn't return these
> information. It can only be used in user defined event trigger function (C
> code).

Thanks for explaining. I did not know the reason why the test is not in event_trigger.sql.

> If we want to add some tests for both already existed and newly added
> ObjectAddresses, we might need to add some test codes in test_ddl_deparse.c.
> What do you think ?

I thought tests for ObjectAddresses should be added to test_ddl_deparse.c, but
it might be bigger because there were many ATExecXXX() functions.
I thought they could be added separately in another thread or patch.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Jul 15, 2022 at 11:39 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, July 15, 2022 11:41 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> Hi,
> >
> > On Fri, Jul 15, 2022 at 03:21:30AM +0000, kuroda.hayato@fujitsu.com wrote:
> > > Sounds good. I grepped ATExecXXX() functions called in ATExecCmd(),
> > > and I confirmed that all returned values have been collected except them.
> > >
> > > While checking test code test about EVENT TRIGGER, I found there were
> > > no tests related with partitions in that.
> > > How about adding them?
> >
> > Agreed.  It would be good to track down what changes once those
> > ObjectAddresses are collected.
>
> Thanks for having a look. It was a bit difficult to add a test for this.
> Because we currently don't have a user function which can return these
> collected ObjectAddresses for ALTER TABLE. And It seems we don't have tests for
> already collected ObjectAddresses as well :(
>
> The collected ObjectAddresses is in
> "currentEventTriggerState->currentCommand->d.alterTable.subcmds.address" while
> the public function pg_event_trigger_ddl_commands doesn't return these
> information. It can only be used in user defined event trigger function (C
> code).
>
> If we want to add some tests for both already existed and newly added
> ObjectAddresses, we might need to add some test codes in test_ddl_deparse.c.
> What do you think ?
>

Right. But, I noticed that get_altertable_subcmdtypes() doesn't handle
AT_AttachPartition or AT_DetachPartition. We can handle those and at
least have a test for those in test_ddl_deparse\sql\slter_table.sql. I
know it is not directly related to your patch but that way we will at
least have some tests for Attach/Detach partition and in the future
when we extend it to test ObjectAddresses of subcommands that would be
handy. Feel free to write a separate patch for the same.

-- 
With Regards,
Amit Kapila.

On Wed, Jul 20, 2022 at 04:36:13PM +0530, Amit Kapila wrote:
> Right. But, I noticed that get_altertable_subcmdtypes() doesn't handle
> AT_AttachPartition or AT_DetachPartition. We can handle those and at
> least have a test for those in test_ddl_deparse\sql\slter_table.sql. I
> know it is not directly related to your patch but that way we will at
> least have some tests for Attach/Detach partition and in the future
> when we extend it to test ObjectAddresses of subcommands that would be
> handy. Feel free to write a separate patch for the same.

Yeah, that could be a separate patch.  On top of that, what about
reworking get_altertable_subcmdtypes() so as it returns one row for
each CollectedCommand, as of (type text, address text)?  We have
already getObjectDescription() to transform the ObjectAddress from the
collected command to a proper string.
--
Michael

On Thu, Jul 21, 2022 at 11:53 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jul 20, 2022 at 04:36:13PM +0530, Amit Kapila wrote:
> > Right. But, I noticed that get_altertable_subcmdtypes() doesn't handle
> > AT_AttachPartition or AT_DetachPartition. We can handle those and at
> > least have a test for those in test_ddl_deparse\sql\slter_table.sql. I
> > know it is not directly related to your patch but that way we will at
> > least have some tests for Attach/Detach partition and in the future
> > when we extend it to test ObjectAddresses of subcommands that would be
> > handy. Feel free to write a separate patch for the same.
>
> Yeah, that could be a separate patch.  On top of that, what about
> reworking get_altertable_subcmdtypes() so as it returns one row for
> each CollectedCommand, as of (type text, address text)?  We have
> already getObjectDescription() to transform the ObjectAddress from the
> collected command to a proper string.
>

Yeah, that would be a good idea but I think instead of changing
get_altertable_subcmdtypes(), can we have a new function say
get_altertable_subcmdinfo() that returns additional information from
address. The other alternative could be that instead of returning the
address as a string, we can return some fields as a set of records
(one row for each subcommand) as we do in
pg_event_trigger_ddl_commands().

-- 
With Regards,
Amit Kapila.

On Fri, Jul 22, 2022 at 02:26:02PM +0530, Amit Kapila wrote:
> Yeah, that would be a good idea but I think instead of changing
> get_altertable_subcmdtypes(), can we have a new function say
> get_altertable_subcmdinfo() that returns additional information from
> address. The other alternative could be that instead of returning the
> address as a string, we can return some fields as a set of records
> (one row for each subcommand) as we do in
> pg_event_trigger_ddl_commands().

Changing get_altertable_subcmdtypes() to return a set of rows made of
(subcommand, object description) is what I actually meant upthread as
it feels natural given a CollectedCommand in input, and as
pg_event_trigger_ddl_commands() only gives access to a set of
CollectedCommands.  This is also a test module so
there is no issue in changing the existing function definitions.

But your point would be to have a new function that takes in input a
CollectedATSubcmd, returning back the object address or its
description?  How would you make sure that a subcommand maps to a
correct object address?
--
Michael

On Sat, Jul 23, 2022 at 05:44:28PM +0900, Michael Paquier wrote:
> Changing get_altertable_subcmdtypes() to return a set of rows made of
> (subcommand, object description) is what I actually meant upthread as
> it feels natural given a CollectedCommand in input, and as
> pg_event_trigger_ddl_commands() only gives access to a set of
> CollectedCommands.  This is also a test module so
> there is no issue in changing the existing function definitions.
>
> But your point would be to have a new function that takes in input a
> CollectedATSubcmd, returning back the object address or its
> description?  How would you make sure that a subcommand maps to a
> correct object address?

FWIW, I was thinking about something among the lines of 0002 on top of
Hou's patch.
--
Michael

On Sat, Jul 23, 2022 at 4:28 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Jul 23, 2022 at 05:44:28PM +0900, Michael Paquier wrote:
> > Changing get_altertable_subcmdtypes() to return a set of rows made of
> > (subcommand, object description) is what I actually meant upthread as
> > it feels natural given a CollectedCommand in input, and as
> > pg_event_trigger_ddl_commands() only gives access to a set of
> > CollectedCommands.  This is also a test module so
> > there is no issue in changing the existing function definitions.
> >
> > But your point would be to have a new function that takes in input a
> > CollectedATSubcmd, returning back the object address or its
> > description?  How would you make sure that a subcommand maps to a
> > correct object address?
>
> FWIW, I was thinking about something among the lines of 0002 on top of
> Hou's patch.
>

What I intended to say is similar to what you have done in the patch
but in a new function. OTOH, your point that it is okay to change
function signature/name in the test module seems reasonable to me.

-- 
With Regards,
Amit Kapila.

On Mon, Jul 25, 2022 at 08:42:18AM +0530, Amit Kapila wrote:
> What I intended to say is similar to what you have done in the patch
> but in a new function. OTOH, your point that it is okay to change
> function signature/name in the test module seems reasonable to me.

Thanks.  Let's do with the function change then.  As introduced
orginally in b488c58, it returns an array that gets just unnested
once, so I'd like to think that it had better be a SRF from the
start but things are what they are.
--
Michael

On Saturday, July 23, 2022 6:58 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Jul 23, 2022 at 05:44:28PM +0900, Michael Paquier wrote:
> > Changing get_altertable_subcmdtypes() to return a set of rows made of
> > (subcommand, object description) is what I actually meant upthread as
> > it feels natural given a CollectedCommand in input, and as
> > pg_event_trigger_ddl_commands() only gives access to a set of
> > CollectedCommands.  This is also a test module so there is no issue in
> > changing the existing function definitions.
> >
> > But your point would be to have a new function that takes in input a
> > CollectedATSubcmd, returning back the object address or its
> > description?  How would you make sure that a subcommand maps to a
> > correct object address?
>
> FWIW, I was thinking about something among the lines of 0002 on top of Hou's
> patch.

Thanks for the patches. The patches look good to me.

BTW, while reviewing it, I found there are some more subcommands that the
get_altertable_subcmdtypes() doesn't handle(e.g., ADD/DROP/SET IDENTITY and re ADD
STAT). Shall we fix them all while on it ?

Attach a minor patch to fix those which is based on the v2 patch set.

Best regards,
Hou zj

On 2022-Jul-25, houzj.fnst@fujitsu.com wrote:

> BTW, while reviewing it, I found there are some more subcommands that the
> get_altertable_subcmdtypes() doesn't handle(e.g., ADD/DROP/SET IDENTITY and re ADD
> STAT). Shall we fix them all while on it ?
> 
> Attach a minor patch to fix those which is based on the v2 patch set.

Yeah, I suppose these are all commands that were added after the last
serious round of event trigger hacking, so it would be good to have
everything on board.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)

On Mon, Jul 25, 2022 at 09:25:07AM +0000, houzj.fnst@fujitsu.com wrote:
> BTW, while reviewing it, I found there are some more subcommands that the
> get_altertable_subcmdtypes() doesn't handle(e.g., ADD/DROP/SET IDENTITY and re ADD
> STAT). Shall we fix them all while on it ?
>
> Attach a minor patch to fix those which is based on the v2 patch set.

@@ -300,6 +300,18 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
[ ... ]
    default:
        strtype = "unrecognized";
        break;

Removing the "default" clause would help here as we would get compiler
warnings if there is anything missing.  One way to do things is to set
strtype to NULL before going through the switch and have a failsafe as
some commands are internal so they may not be worth adding to the
output.
--
Michael

On Monday, July 25, 2022 6:26 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jul 25, 2022 at 09:25:07AM +0000, houzj.fnst@fujitsu.com wrote:
> > BTW, while reviewing it, I found there are some more subcommands that
> > the
> > get_altertable_subcmdtypes() doesn't handle(e.g., ADD/DROP/SET
> > IDENTITY and re ADD STAT). Shall we fix them all while on it ?
> >
> > Attach a minor patch to fix those which is based on the v2 patch set.
>
> @@ -300,6 +300,18 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
> [ ... ]
>     default:
>         strtype = "unrecognized";
>         break;
>
> Removing the "default" clause would help here as we would get compiler
> warnings if there is anything missing.  One way to do things is to set strtype to
> NULL before going through the switch and have a failsafe as some commands
> are internal so they may not be worth adding to the output.

Thanks for the suggestion. I have removed the default and found some missed
subcommands in 0003 patch. Attach the new version patch here
(The 0001 and 0002 is unchanged).

Best regards,
Hou zj

On Tue, Jul 26, 2022 at 01:00:41PM +0000, houzj.fnst@fujitsu.com wrote:
> Thanks for the suggestion. I have removed the default and found some missed
> subcommands in 0003 patch. Attach the new version patch here
> (The 0001 and 0002 is unchanged).

I have reviewed what you have here, and I found that the change is too
timid, with a coverage of 32% for test_ddl_deparse.  Attached is an
updated patch, that provides coverage for the most obvious cases I
could see in tablecmds.c, bringing the coverage to 64% here.

Some cases are straight-forward, like the four cases for RLS or the
three subcases for RelOptions (where we'd better return an address
even if doing doing for the replace case).  Some cases that I have not
included here would need more thoughts, like constraint validation and
drop or even SET ACCESS METHOD, so I have discarded for now all the
cases where we don't (or cannot) report properly an ObjectAddress
yet.

There is also a fancy case with DROP COLUMN, where we get an
ObjectAddress referring to the column already dropped, aka roughly a
".....pg_dropped.N.....", and it is not like we should switch to only
a reference of the table here because we want to know the name of the
column dropped.  I have discarded this last one as well, for now.

All that could be expanded in more patches (triggers are an easy one),
but what I have here is already a good cut.
--
Michael

On Saturday, July 30, 2022 3:15 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, Jul 26, 2022 at 01:00:41PM +0000, houzj.fnst@fujitsu.com wrote:
> > Thanks for the suggestion. I have removed the default and found some
> > missed subcommands in 0003 patch. Attach the new version patch here
> > (The 0001 and 0002 is unchanged).
>
> I have reviewed what you have here, and I found that the change is too timid,
> with a coverage of 32% for test_ddl_deparse.  Attached is an updated patch, that
> provides coverage for the most obvious cases I could see in tablecmds.c,
> bringing the coverage to 64% here.

Thanks ! the patch looks better now.

> Some cases are straight-forward, like the four cases for RLS or the three
> subcases for RelOptions (where we'd better return an address even if doing
> doing for the replace case).

I am not against returning the objaddr for cases related to RLS and RelOption.
But just to confirm, do you have a use case to use the returned address(relation itself)
for RLS or RelOptions in event trigger ? I asked this because when I tried to
deparse the subcommand of ALTER TABLE. It seems enough to use the information
inside the parse tree to deparse the RLS and RelOptions related subcommands.

Best regards,
Hou Zhijie

On Sat, Jul 30, 2022 at 01:13:52PM +0000, houzj.fnst@fujitsu.com wrote:
> I am not against returning the objaddr for cases related to RLS and RelOption.
> But just to confirm, do you have a use case to use the returned address(relation itself)
> for RLS or RelOptions in event trigger ? I asked this because when I tried to
> deparse the subcommand of ALTER TABLE. It seems enough to use the information
> inside the parse tree to deparse the RLS and RelOptions related subcommands.

You are right here, there is little point in returning the relation
itself.  I have removed these modifications, added a couple of extra
commands for some extra coverage, and applied all that.  I have
finished by splitting the extension of test_ddl_deparse/ and the
addition of ObjectAddress for the attach/detach into their own commit,
mainly for clarity.
--
Michael

On Sunday, July 31, 2022 12:12 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Jul 30, 2022 at 01:13:52PM +0000, houzj.fnst@fujitsu.com wrote:
> > I am not against returning the objaddr for cases related to RLS and
> RelOption.
> > But just to confirm, do you have a use case to use the returned
> > address(relation itself) for RLS or RelOptions in event trigger ? I
> > asked this because when I tried to deparse the subcommand of ALTER
> > TABLE. It seems enough to use the information inside the parse tree to
> deparse the RLS and RelOptions related subcommands.
>
> You are right here, there is little point in returning the relation itself.  I have
> removed these modifications, added a couple of extra commands for some
> extra coverage, and applied all that.  I have finished by splitting the extension
> of test_ddl_deparse/ and the addition of ObjectAddress for the attach/detach
> into their own commit, mainly for clarity.

Thanks!

Best regards,
Hou Zhijie