[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
Attachments:
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
event-trigger-ts-config-map.patchtext/x-patch; charset=US-ASCII; name=event-trigger-ts-config-map.patchDownload
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:
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.
--
MichaelAfter 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
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
--
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-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.
--
MichaelAfter 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
Attachments:
event-trigger-ts-config-map-v2.patchtext/x-patch; charset=US-ASCII; name=event-trigger-ts-config-map-v2.patchDownload
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 0000000000..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 0000000000..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;
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