Re: [HACKERS] [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION
Artur, * Artur Zakirov (a.zaki...@postgrespro.ru) wrote: > 2016-12-21 20:34 GMT+03:00 Stephen Frost : > > 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 signature.asc Description: Digital signature
Re: [HACKERS] [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION
Thank you for your comments, Stephen. 2016-12-21 20:34 GMT+03:00 Stephen Frost : > > 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 diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c index b24011371c..2b84848cf5 100644 --- a/src/backend/commands/tsearchcmds.c +++ b/src/backend/commands/tsearchcmds.c @@ -1215,10 +1215,10 @@ AlterTSConfiguration(AlterTSConfigurationStmt *stmt) /* Update dependencies */ makeConfigurationDependencies(tup, true, relMap); - InvokeObjectPostAlterHook(TSConfigMapRelationId, + InvokeObjectPostAlterHook(TSConfigRelationId, HeapTupleGetOid(tup), 0); - ObjectAddressSet(address, TSConfigMapRelationId, cfgId); + ObjectAddressSet(address, TSConfigRelationId, cfgId); heap_close(relMap, RowExclusiveLock); diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index fd4eff4907..b7a4c8e531 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1479,7 +1479,8 @@ ProcessUtilitySlow(ParseState *pstate, break; case T_AlterTSConfigurationStmt: -address = AlterTSConfiguration((AlterTSConfigurationStmt *) parsetree); +AlterTSConfiguration((AlterTSConfigurationStmt *) parsetree); +commandCollected = true; break; case T_AlterTableMoveAllStmt: diff --git a/src/test/modules/test_ddl_deparse/Makefile b/src/test/modules/test_ddl_deparse/Makefile index 8ea6f39afd..3a57a95c84 100644 --- a/src/test/modules/test_ddl_deparse/Makefile +++ b/src/test/modules/test_ddl_deparse/Makefile @@ -23,6 +23,7 @@ REGRESS = test_ddl_deparse \ comment_on \ alter_function \ alter_sequence \ + alter_ts_config \ alter_type_enum \ opfamily \ defprivs \ diff --git a/src/test/modules/test_ddl_deparse/expected/alter_ts_config.out b/src/test/modules/test_ddl_deparse/expected/alter_ts_config.out new file mode 100644 index 00..afc352fc5f --- /dev/null +++ b/src/test/modules/test_ddl_deparse/expected/alter_ts_config.out @@ -0,0 +1,8 @@ +-- +-- ALTER TEXT SEARCH CONFIGURATION +-- +CREATE TEXT SEARCH CONFIGURATION en (copy=english); +NOTICE: DDL test: type simple, tag CREATE TEXT SEARCH CONFIGURATION +ALTER TEXT SEARCH CONFIGURATION en + ALTER MAPPING FOR host, email, url, sfloat WITH simple; +NOTICE: DDL test: type alter text search configuration, tag ALTER TEXT SEARCH CONFIGURATION diff --git a/src/test/modules/test_ddl_deparse/sql/alter_ts_config.sql b/src/test/modules/test_ddl_deparse/sql/alter_ts_config.sql new file mode 100644 index 00..ac13e21dda --- /dev/null +++ b/src/test/modules/test_ddl_deparse/sql/alter_ts_config.sql @@ -0,0 +1,8 @@ +-- +-- ALTER TEXT SEARCH CONFIGURATION +-- + +CREATE TEXT SEARCH CONFIGURATION en (copy=english); + +ALTER TEXT SEARCH CONFIGURATION en + ALTER MAPPING FOR host, email, url, sfloat WITH simple; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION
Artur, * Artur Zakirov (a.zaki...@postgrespro.ru) wrote: > 2016-11-19 21:28 GMT+03:00 Michael Paquier : > > On Thu, Nov 17, 2016 at 1:17 PM, Alvaro Herrera > > 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 signature.asc Description: Digital signature
Re: [HACKERS] [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION
2016-11-28 21:32 GMT+03:00 Robert Haas : > > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION
On Sun, Nov 27, 2016 at 1:59 PM, Artur Zakirov wrote: > Thank you for answers. > > 2016-11-19 21:28 GMT+03:00 Michael Paquier : >> On Thu, Nov 17, 2016 at 1:17 PM, Alvaro Herrera >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION
Thank you for answers. 2016-11-19 21:28 GMT+03:00 Michael Paquier : > On Thu, Nov 17, 2016 at 1:17 PM, Alvaro Herrera > 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 diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c index b240113..2b84848 100644 --- a/src/backend/commands/tsearchcmds.c +++ b/src/backend/commands/tsearchcmds.c @@ -1215,10 +1215,10 @@ AlterTSConfiguration(AlterTSConfigurationStmt *stmt) /* Update dependencies */ makeConfigurationDependencies(tup, true, relMap); - InvokeObjectPostAlterHook(TSConfigMapRelationId, + InvokeObjectPostAlterHook(TSConfigRelationId, HeapTupleGetOid(tup), 0); - ObjectAddressSet(address, TSConfigMapRelationId, cfgId); + ObjectAddressSet(address, TSConfigRelationId, cfgId); heap_close(relMap, RowExclusiveLock); diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index f50ce40..1217d1a 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1477,7 +1477,8 @@ ProcessUtilitySlow(ParseState *pstate, break; case T_AlterTSConfigurationStmt: -address = AlterTSConfiguration((AlterTSConfigurationStmt *) parsetree); +AlterTSConfiguration((AlterTSConfigurationStmt *) parsetree); +commandCollected = true; break; case T_AlterTableMoveAllStmt: -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION
On Thu, Nov 17, 2016 at 1:17 PM, Alvaro Herrera 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
