Обсуждение: [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION

Hello hackers,

I attached the test extension. It just creates event trigger on
ddl_command_end. And pg_catalog.pg_event_trigger_ddl_commands() is
executed from extension's C-function handler.

The pg_event_trigger_ddl_commands() returns the error:

ERROR:  unrecognized object class: 3603

if I try to execute the following commands:

=> create text search CONFIGURATION en (copy=english);

=> alter text search CONFIGURATION en ALTER MAPPING for host, email,
url, sfloat with simple;
ERROR:  unrecognized object class: 3603
CONTEXT:  SQL statement "SELECT objid, UPPER(object_type), object_identity,
   UPPER(command_tag)
   FROM pg_catalog.pg_event_trigger_ddl_commands()"

This error is raised from getObjectClass() function. It seems that we
need new OCLASS_TSCONFIG_MAP object class.

Is this a bug? Or it wasn't added intentionally?

Thanks in advance.

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Artur Zakirov wrote:
> Hello hackers,
> 
> I attached the test extension. It just creates event trigger on
> ddl_command_end. And pg_catalog.pg_event_trigger_ddl_commands() is executed
> from extension's C-function handler.
> 
> The pg_event_trigger_ddl_commands() returns the error:
> 
> ERROR:  unrecognized object class: 3603
> 
> if I try to execute the following commands:
> 
> => create text search CONFIGURATION en (copy=english);
> 
> => alter text search CONFIGURATION en ALTER MAPPING for host, email, url,
> sfloat with simple;
> ERROR:  unrecognized object class: 3603
> CONTEXT:  SQL statement "SELECT objid, UPPER(object_type), object_identity,
>   UPPER(command_tag)
>   FROM pg_catalog.pg_event_trigger_ddl_commands()"
> 
> This error is raised from getObjectClass() function. It seems that we need
> new OCLASS_TSCONFIG_MAP object class.
> 
> Is this a bug? Or it wasn't added intentionally?

It's a bug.  You're right that we need to handle the object class
somewhere.  Perhaps I failed to realize that tsconfigs could get
altered.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Nov 17, 2016 at 1:17 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> It's a bug.  You're right that we need to handle the object class
> somewhere.  Perhaps I failed to realize that tsconfigs could get
> altered.

It seems to me that the thing to be careful of here is how a new
OBJECT_TSCONFIGURATIONMAP should use ObjectAddress. It does not seem
that complicated, but it needs some work.
-- 
Michael

Thank you for answers.

2016-11-19 21:28 GMT+03:00 Michael Paquier <michael.paquier@gmail.com>:
> On Thu, Nov 17, 2016 at 1:17 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> It's a bug.  You're right that we need to handle the object class
>> somewhere.  Perhaps I failed to realize that tsconfigs could get
>> altered.
>
> It seems to me that the thing to be careful of here is how a new
> OBJECT_TSCONFIGURATIONMAP should use ObjectAddress. It does not seem
> that complicated, but it needs some work.
> --
> Michael

After some investigation it seems to me that OBJECT_TSCONFIGURATIONMAP
can't be added for pg_ts_config_map. Because this catalog hasn't Oids.

But this bug can be easily fixed (patch attached). I think in
AlterTSConfiguration() TSConfigRelationId should be used instead of
TSConfigMapRelationId. Secondly, in ProcessUtilitySlow() we can use
commandCollected = true. Because configuration entry is added in
EventTriggerCollectAlterTSConfig() into
currentEventTriggerState->commandList.

This patch only fixes the bug. But I think I also can do a patch which
will give pg_ts_config_map entries with
pg_event_trigger_ddl_commands() if someone wants. It can be done using
new entry in the CollectedCommandType structure maybe.

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Sun, Nov 27, 2016 at 1:59 PM, Artur Zakirov <a.zakirov@postgrespro.ru> wrote:
> Thank you for answers.
>
> 2016-11-19 21:28 GMT+03:00 Michael Paquier <michael.paquier@gmail.com>:
>> On Thu, Nov 17, 2016 at 1:17 PM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>>> It's a bug.  You're right that we need to handle the object class
>>> somewhere.  Perhaps I failed to realize that tsconfigs could get
>>> altered.
>>
>> It seems to me that the thing to be careful of here is how a new
>> OBJECT_TSCONFIGURATIONMAP should use ObjectAddress. It does not seem
>> that complicated, but it needs some work.
>> --
>> Michael
>
> After some investigation it seems to me that OBJECT_TSCONFIGURATIONMAP
> can't be added for pg_ts_config_map. Because this catalog hasn't Oids.
>
> But this bug can be easily fixed (patch attached). I think in
> AlterTSConfiguration() TSConfigRelationId should be used instead of
> TSConfigMapRelationId. Secondly, in ProcessUtilitySlow() we can use
> commandCollected = true. Because configuration entry is added in
> EventTriggerCollectAlterTSConfig() into
> currentEventTriggerState->commandList.
>
> This patch only fixes the bug. But I think I also can do a patch which
> will give pg_ts_config_map entries with
> pg_event_trigger_ddl_commands() if someone wants. It can be done using
> new entry in the CollectedCommandType structure maybe.

You might need to add this patch to https://commitfest.postgresql.org/
so that it doesn't get forgotten.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2016-11-28 21:32 GMT+03:00 Robert Haas <robertmhaas@gmail.com>:
>
> You might need to add this patch to https://commitfest.postgresql.org/
> so that it doesn't get forgotten.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

I added the patch to https://commitfest.postgresql.org/12/895/
I added it to the next commitfest. Because we are in the end of the
current commitfest.

-- 
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Artur,

* Artur Zakirov (a.zakirov@postgrespro.ru) wrote:
> 2016-11-19 21:28 GMT+03:00 Michael Paquier <michael.paquier@gmail.com>:
> > On Thu, Nov 17, 2016 at 1:17 PM, Alvaro Herrera
> > <alvherre@2ndquadrant.com> wrote:
> >> It's a bug.  You're right that we need to handle the object class
> >> somewhere.  Perhaps I failed to realize that tsconfigs could get
> >> altered.
> >
> > It seems to me that the thing to be careful of here is how a new
> > OBJECT_TSCONFIGURATIONMAP should use ObjectAddress. It does not seem
> > that complicated, but it needs some work.
> > --
> > Michael
>
> After some investigation it seems to me that OBJECT_TSCONFIGURATIONMAP
> can't be added for pg_ts_config_map. Because this catalog hasn't Oids.

I started looking into this, in part because it's a bug fix.

> But this bug can be easily fixed (patch attached). I think in
> AlterTSConfiguration() TSConfigRelationId should be used instead of
> TSConfigMapRelationId. Secondly, in ProcessUtilitySlow() we can use
> commandCollected = true. Because configuration entry is added in
> EventTriggerCollectAlterTSConfig() into
> currentEventTriggerState->commandList.

We should definitely be using TSConfigRelationId there, as the tuple we
have the OID of is from pg_ts_config, not from pg_ts_config_map.  You're
also right that we need to set commandCollected = true since the
MakConfigurationMapping() and DropConfigurationMapping() functions get
called from AlterTSConfiguration and, as you say, they call
EventTriggerCollectAlterTSConfig().

Looks like the InvokeObjectPostAlterHook() call has been around since
9.3, so we'll need to back-patch it that far, while the other changes go
back to 9.5.

Did you happen to look at adding a regression test for this to
test_ddl_deparse?

> This patch only fixes the bug. But I think I also can do a patch which
> will give pg_ts_config_map entries with
> pg_event_trigger_ddl_commands() if someone wants. It can be done using
> new entry in the CollectedCommandType structure maybe.

While that sounds like a good idea, it seems like it's more a feature
addition rather than a bugfix, no?

Thanks!

Stephen

Thank you for your comments, Stephen.

2016-12-21 20:34 GMT+03:00 Stephen Frost <sfrost@snowman.net>:
>
> Did you happen to look at adding a regression test for this to
> test_ddl_deparse?

Of course. I updated the patch.

>
>> This patch only fixes the bug. But I think I also can do a patch which
>> will give pg_ts_config_map entries with
>> pg_event_trigger_ddl_commands() if someone wants. It can be done using
>> new entry in the CollectedCommandType structure maybe.
>
> While that sounds like a good idea, it seems like it's more a feature
> addition rather than a bugfix, no?
>

Yes, agree with you. It should be added as a separate patch. I think I
will deal with it.


-- 
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Artur,

* Artur Zakirov (a.zakirov@postgrespro.ru) wrote:
> 2016-12-21 20:34 GMT+03:00 Stephen Frost <sfrost@snowman.net>:
> > Did you happen to look at adding a regression test for this to
> > test_ddl_deparse?
>
> Of course. I updated the patch.

I added a few comments and back-patched the relevant bits as discussed.

Thanks for the bug report and patch!

Stephen