public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v25 4/5] Add ALTER INDEX ... ALTER COLLATION ... REFRESH VERSION.
10+ messages / 6 participants
[nested] [flat]
* [PATCH v25 4/5] Add ALTER INDEX ... ALTER COLLATION ... REFRESH VERSION.
@ 2019-12-11 12:54 Julien Rouhaud <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Julien Rouhaud @ 2019-12-11 12:54 UTC (permalink / raw)
This command allows privileged users to specify that the currently installed
collation version, for a specific collation, is binary compatible with the one
that was installed when the specified index was built. This provides a way to
clear warnings about potentially corrupted indexes without having to use
REINDEX.
Author: Julien Rouhaud
Reviewed-by: Laurenz Albe, Thomas Munro and Peter Eisentraut
Discussion: https://postgr.es/m/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com
---
doc/src/sgml/ref/alter_index.sgml | 17 +++++++
src/backend/catalog/index.c | 2 +-
src/backend/commands/tablecmds.c | 46 +++++++++++++++++++
src/backend/nodes/copyfuncs.c | 1 +
src/backend/parser/gram.y | 8 ++++
src/bin/psql/tab-complete.c | 27 ++++++++++-
src/include/catalog/index.h | 3 ++
src/include/nodes/parsenodes.h | 4 +-
.../regress/expected/collate.icu.utf8.out | 20 ++++++++
src/test/regress/sql/collate.icu.utf8.sql | 11 +++++
10 files changed, 136 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/ref/alter_index.sgml b/doc/src/sgml/ref/alter_index.sgml
index a5e3b06ee4..03355164b3 100644
--- a/doc/src/sgml/ref/alter_index.sgml
+++ b/doc/src/sgml/ref/alter_index.sgml
@@ -25,6 +25,7 @@ ALTER INDEX [ IF EXISTS ] <replaceable class="parameter">name</replaceable> RENA
ALTER INDEX [ IF EXISTS ] <replaceable class="parameter">name</replaceable> SET TABLESPACE <replaceable class="parameter">tablespace_name</replaceable>
ALTER INDEX <replaceable class="parameter">name</replaceable> ATTACH PARTITION <replaceable class="parameter">index_name</replaceable>
ALTER INDEX <replaceable class="parameter">name</replaceable> DEPENDS ON EXTENSION <replaceable class="parameter">extension_name</replaceable>
+ALTER INDEX <replaceable class="parameter">name</replaceable> ALTER COLLATION <replaceable class="parameter">collation_name</replaceable> REFRESH VERSION
ALTER INDEX [ IF EXISTS ] <replaceable class="parameter">name</replaceable> SET ( <replaceable class="parameter">storage_parameter</replaceable> [= <replaceable class="parameter">value</replaceable>] [, ... ] )
ALTER INDEX [ IF EXISTS ] <replaceable class="parameter">name</replaceable> RESET ( <replaceable class="parameter">storage_parameter</replaceable> [, ... ] )
ALTER INDEX [ IF EXISTS ] <replaceable class="parameter">name</replaceable> ALTER [ COLUMN ] <replaceable class="parameter">column_number</replaceable>
@@ -112,6 +113,22 @@ ALTER INDEX ALL IN TABLESPACE <replaceable class="parameter">name</replaceable>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>ALTER COLLATION <replaceable class="parameter">collation_name</replaceable> REFRESH VERSION</literal></term>
+ <listitem>
+ <para>
+ This command declares that the index is compatible with the currently
+ installed version of a collation that determines its order. It is used
+ to silence warnings caused by collation version incompatibilities and
+ should be issued only if the collation ordering is known not to have
+ changed since the index was last built. Be aware that incorrect use of
+ this command can hide index corruption. If you don't know whether a
+ collation's definition has changed, using <xref linkend="sql-reindex"/>
+ is a safe alternative.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>SET ( <replaceable class="parameter">storage_parameter</replaceable> [= <replaceable class="parameter">value</replaceable>] [, ... ] )</literal></term>
<listitem>
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index e85aeb5502..ef2b896b6f 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3156,7 +3156,7 @@ index_build(Relation heapRelation,
SetUserIdAndSecContext(save_userid, save_sec_context);
}
-static char *
+char *
index_force_collation_version(const ObjectAddress *otherObject,
const char *version,
void *userdata)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f79044f39f..cdf3cbf5b1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -93,6 +93,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/partcache.h"
+#include "utils/pg_locale.h"
#include "utils/relcache.h"
#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
@@ -556,6 +557,7 @@ static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
Relation partitionTbl);
static List *GetParentedForeignKeyRefs(Relation partition);
static void ATDetachCheckNoForeignKeyRefs(Relation partition);
+static void ATExecAlterCollationRefreshVersion(Relation rel, List *coll);
/* ----------------------------------------------------------------
@@ -3926,6 +3928,10 @@ AlterTableGetLockLevel(List *cmds)
cmd_lockmode = AccessShareLock;
break;
+ case AT_AlterCollationRefreshVersion:
+ cmd_lockmode = AccessExclusiveLock;
+ break;
+
default: /* oops */
elog(ERROR, "unrecognized alter table type: %d",
(int) cmd->subtype);
@@ -4093,6 +4099,12 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
/* This command never recurses */
pass = AT_PASS_MISC;
break;
+ case AT_AlterCollationRefreshVersion: /* ALTER COLLATION ... REFRESH
+ * VERSION */
+ ATSimplePermissions(rel, ATT_INDEX);
+ /* This command never recurses */
+ pass = AT_PASS_MISC;
+ break;
case AT_SetStorage: /* ALTER COLUMN SET STORAGE */
ATSimplePermissions(rel, ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
@@ -4659,6 +4671,11 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
ATExecDetachPartition(rel, ((PartitionCmd *) cmd->def)->name);
break;
+ case AT_AlterCollationRefreshVersion:
+ /* ATPrepCmd ensured it must be an index */
+ Assert(rel->rd_rel->relkind == RELKIND_INDEX);
+ ATExecAlterCollationRefreshVersion(rel, cmd->object);
+ break;
default: /* oops */
elog(ERROR, "unrecognized alter table type: %d",
(int) cmd->subtype);
@@ -17477,3 +17494,32 @@ ATDetachCheckNoForeignKeyRefs(Relation partition)
table_close(rel, NoLock);
}
}
+
+/* Execute an ALTER INDEX ... ALTER COLLATION ... REFRESH VERSION
+ *
+ * This override an existing dependency on a specific collation for a specific
+ * index to depend on the current collation version.
+ */
+static void
+ATExecAlterCollationRefreshVersion(Relation rel, List *coll)
+{
+ ObjectAddress object;
+ NewCollationVersionDependency forced_dependency;
+
+ Assert(coll != NIL);
+ forced_dependency.oid = get_collation_oid(coll, false);
+
+ /* Retrieve the current version for the CURRENT VERSION case. */
+ Assert(OidIsValid(forced_dependency.oid));
+ forced_dependency.version =
+ get_collation_version_for_oid(forced_dependency.oid);
+
+ object.classId = RelationRelationId;
+ object.objectId = rel->rd_id;
+ object.objectSubId = 0;
+ visitDependentObjects(&object, &index_force_collation_version,
+ &forced_dependency);
+
+ /* Invalidate the index relcache */
+ CacheInvalidateRelcache(rel);
+}
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 5cbbe3ba2e..ebf9118f4f 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3217,6 +3217,7 @@ _copyAlterTableCmd(const AlterTableCmd *from)
COPY_SCALAR_FIELD(subtype);
COPY_STRING_FIELD(name);
+ COPY_NODE_FIELD(object);
COPY_SCALAR_FIELD(num);
COPY_NODE_FIELD(newowner);
COPY_NODE_FIELD(def);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index c43b16037e..68934bfeb1 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2594,6 +2594,14 @@ alter_table_cmd:
n->subtype = AT_NoForceRowSecurity;
$$ = (Node *)n;
}
+ /* ALTER INDEX <name> ALTER COLLATION ... REFRESH VERSION */
+ | ALTER COLLATION any_name REFRESH VERSION_P
+ {
+ AlterTableCmd *n = makeNode(AlterTableCmd);
+ n->subtype = AT_AlterCollationRefreshVersion;
+ n->object = $3;
+ $$ = (Node *)n;
+ }
| alter_generic_options
{
AlterTableCmd *n = makeNode(AlterTableCmd);
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index eb018854a5..ec0ef1feae 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -45,6 +45,7 @@
#include "catalog/pg_am_d.h"
#include "catalog/pg_class_d.h"
+#include "catalog/pg_collation_d.h"
#include "common.h"
#include "libpq-fe.h"
#include "pqexpbuffer.h"
@@ -814,6 +815,20 @@ static const SchemaQuery Query_for_list_of_statistics = {
" (SELECT tgrelid FROM pg_catalog.pg_trigger "\
" WHERE pg_catalog.quote_ident(tgname)='%s')"
+/* the silly-looking length condition is just to eat up the current word */
+#define Query_for_list_of_colls_for_one_index \
+" SELECT DISTINCT pg_catalog.quote_ident(coll.collname) " \
+" FROM pg_catalog.pg_depend d, pg_catalog.pg_collation coll, " \
+" pg_catalog.pg_class c" \
+" WHERE (%d = pg_catalog.length('%s'))" \
+" AND d.refclassid = " CppAsString2(CollationRelationId) \
+" AND d.refobjid = coll.oid " \
+" AND d.classid = " CppAsString2(RelationRelationId) \
+" AND d.objid = c.oid " \
+" AND c.relkind = " CppAsString2(RELKIND_INDEX) \
+" AND pg_catalog.pg_table_is_visible(c.oid) " \
+" AND c.relname = '%s'"
+
#define Query_for_list_of_ts_configurations \
"SELECT pg_catalog.quote_ident(cfgname) FROM pg_catalog.pg_ts_config "\
" WHERE substring(pg_catalog.quote_ident(cfgname),1,%d)='%s'"
@@ -1709,7 +1724,8 @@ psql_completion(const char *text, int start, int end)
/* ALTER INDEX <name> */
else if (Matches("ALTER", "INDEX", MatchAny))
COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME TO", "SET",
- "RESET", "ATTACH PARTITION", "DEPENDS", "NO DEPENDS");
+ "RESET", "ATTACH PARTITION", "DEPENDS", "NO DEPENDS",
+ "ALTER COLLATION");
else if (Matches("ALTER", "INDEX", MatchAny, "ATTACH"))
COMPLETE_WITH("PARTITION");
else if (Matches("ALTER", "INDEX", MatchAny, "ATTACH", "PARTITION"))
@@ -1759,6 +1775,15 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH("ON EXTENSION");
else if (Matches("ALTER", "INDEX", MatchAny, "DEPENDS"))
COMPLETE_WITH("ON EXTENSION");
+ /* ALTER INDEX <name> ALTER COLLATION */
+ else if (Matches("ALTER", "INDEX", MatchAny, "ALTER", "COLLATION"))
+ {
+ completion_info_charp = prev4_wd;
+ COMPLETE_WITH_QUERY(Query_for_list_of_colls_for_one_index);
+ }
+ /* ALTER INDEX <name> ALTER COLLATION <name> */
+ else if (Matches("ALTER", "INDEX", MatchAny, "ALTER", "COLLATION", MatchAny))
+ COMPLETE_WITH("REFRESH VERSION");
/* ALTER LANGUAGE <name> */
else if (Matches("ALTER", "LANGUAGE", MatchAny))
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 9b4de26514..46d5df1613 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -123,6 +123,9 @@ extern void FormIndexDatum(IndexInfo *indexInfo,
extern void index_check_collation_versions(Oid relid);
+extern char *index_force_collation_version(const ObjectAddress *otherObject,
+ const char *version,
+ void *userdata);
extern void index_force_collation_versions(Oid indexid, Oid coll,
char *version);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 7a23fb7529..423f33b72c 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1847,7 +1847,8 @@ typedef enum AlterTableType
AT_DetachPartition, /* DETACH PARTITION */
AT_AddIdentity, /* ADD IDENTITY */
AT_SetIdentity, /* SET identity column options */
- AT_DropIdentity /* DROP IDENTITY */
+ AT_DropIdentity, /* DROP IDENTITY */
+ AT_AlterCollationRefreshVersion /* ALTER COLLATION ... REFRESH VERSION */
} AlterTableType;
typedef struct ReplicaIdentityStmt
@@ -1863,6 +1864,7 @@ typedef struct AlterTableCmd /* one subcommand of an ALTER TABLE */
AlterTableType subtype; /* Type of table alteration to apply */
char *name; /* column, constraint, or trigger to act on,
* or tablespace */
+ List *object; /* collation to act on if it's a collation */
int16 num; /* attribute number for columns referenced by
* number */
RoleSpec *newowner;
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index db386c1b09..adc1dddda7 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2054,6 +2054,26 @@ SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version';
-------
(0 rows)
+-- Test ALTER INDEX name ALTER COLLATION name REFRESH VERSION
+UPDATE pg_depend SET refobjversion = 'not a version'
+WHERE refclassid = 'pg_collation'::regclass
+AND objid::regclass::text = 'icuidx17_part'
+AND refobjversion IS NOT NULL;
+SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version';
+ objid
+---------------
+ icuidx17_part
+(1 row)
+
+ALTER INDEX icuidx17_part ALTER COLLATION "en-x-icu" REFRESH VERSION;
+SELECT objid::regclass, refobjversion = 'not a version' AS ver FROM pg_depend
+WHERE refclassid = 'pg_collation'::regclass
+AND objid::regclass::text = 'icuidx17_part';
+ objid | ver
+---------------+-----
+ icuidx17_part | f
+(1 row)
+
-- cleanup
RESET search_path;
SET client_min_messages TO warning;
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index e93530af55..b3a75b29e6 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -823,6 +823,17 @@ VACUUM FULL collate_part_1;
SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version';
+-- Test ALTER INDEX name ALTER COLLATION name REFRESH VERSION
+UPDATE pg_depend SET refobjversion = 'not a version'
+WHERE refclassid = 'pg_collation'::regclass
+AND objid::regclass::text = 'icuidx17_part'
+AND refobjversion IS NOT NULL;
+SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version';
+ALTER INDEX icuidx17_part ALTER COLLATION "en-x-icu" REFRESH VERSION;
+SELECT objid::regclass, refobjversion = 'not a version' AS ver FROM pg_depend
+WHERE refclassid = 'pg_collation'::regclass
+AND objid::regclass::text = 'icuidx17_part';
+
-- cleanup
RESET search_path;
SET client_min_messages TO warning;
--
2.20.1
--lrZ03NoBR/3+SXJZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename="v25-0005-doc-Add-Collation-Versions-section.patch"
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING
@ 2024-02-07 20:22 Laurenz Albe <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Laurenz Albe @ 2024-02-07 20:22 UTC (permalink / raw)
To: James Coleman <[email protected]>; pgsql-hackers
On Wed, 2024-02-07 at 15:12 -0500, James Coleman wrote:
> We recently noticed some behavior that seems reasonable but also
> surprised our engineers based on the docs.
>
> If we have this setup:
> create table items(i int);
> insert into items(i) values (1);
> create publication test_pub for all tables;
>
> Then when we:
> delete from items where i = 1;
>
> we get:
> ERROR: cannot delete from table "items" because it does not have a
> replica identity and publishes deletes
> HINT: To enable deleting from the table, set REPLICA IDENTITY using
> ALTER TABLE.
>
> Fair enough. But if we do this:
> alter table items replica identity nothing;
>
> because the docs [1] say that NOTHING means "Records no information
> about the old row." We still get the same error when we try the DELETE
> again.
Well, "REPLICA IDENTITY NOTHING" is the same as "has no replica identity".
So is "REPLICA IDENTITY DEFAULT" if there is no primary key, or
"REPLICA IDENTITY USING INDEX ..." if the index is dropped.
See "pg_class": the column "relreplident" is not nullable.
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING
@ 2024-02-07 22:03 James Coleman <[email protected]>
parent: Laurenz Albe <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: James Coleman @ 2024-02-07 22:03 UTC (permalink / raw)
To: Laurenz Albe <[email protected]>; +Cc: pgsql-hackers
On Wed, Feb 7, 2024 at 3:22 PM Laurenz Albe <[email protected]> wrote:
>
> On Wed, 2024-02-07 at 15:12 -0500, James Coleman wrote:
> > We recently noticed some behavior that seems reasonable but also
> > surprised our engineers based on the docs.
> >
> > If we have this setup:
> > create table items(i int);
> > insert into items(i) values (1);
> > create publication test_pub for all tables;
> >
> > Then when we:
> > delete from items where i = 1;
> >
> > we get:
> > ERROR: cannot delete from table "items" because it does not have a
> > replica identity and publishes deletes
> > HINT: To enable deleting from the table, set REPLICA IDENTITY using
> > ALTER TABLE.
> >
> > Fair enough. But if we do this:
> > alter table items replica identity nothing;
> >
> > because the docs [1] say that NOTHING means "Records no information
> > about the old row." We still get the same error when we try the DELETE
> > again.
>
> Well, "REPLICA IDENTITY NOTHING" is the same as "has no replica identity".
> So is "REPLICA IDENTITY DEFAULT" if there is no primary key, or
> "REPLICA IDENTITY USING INDEX ..." if the index is dropped.
>
> See "pg_class": the column "relreplident" is not nullable.
Right, I think the confusing point for us is that the docs for NOTHING
("Records no information about the old row") imply you can decide you
don't have to record anything if you don't want to do so, but the
publication feature is effectively overriding that and asserting that
you can't make that choice.
Regards,
James Coleman
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING
@ 2024-02-07 23:04 Peter Smith <[email protected]>
parent: James Coleman <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Peter Smith @ 2024-02-07 23:04 UTC (permalink / raw)
To: James Coleman <[email protected]>; +Cc: Laurenz Albe <[email protected]>; pgsql-hackers
On Thu, Feb 8, 2024 at 9:04 AM James Coleman <[email protected]> wrote:
>
> On Wed, Feb 7, 2024 at 3:22 PM Laurenz Albe <[email protected]> wrote:
> >
> > On Wed, 2024-02-07 at 15:12 -0500, James Coleman wrote:
> > > We recently noticed some behavior that seems reasonable but also
> > > surprised our engineers based on the docs.
> > >
> > > If we have this setup:
> > > create table items(i int);
> > > insert into items(i) values (1);
> > > create publication test_pub for all tables;
> > >
> > > Then when we:
> > > delete from items where i = 1;
> > >
> > > we get:
> > > ERROR: cannot delete from table "items" because it does not have a
> > > replica identity and publishes deletes
> > > HINT: To enable deleting from the table, set REPLICA IDENTITY using
> > > ALTER TABLE.
> > >
> > > Fair enough. But if we do this:
> > > alter table items replica identity nothing;
> > >
> > > because the docs [1] say that NOTHING means "Records no information
> > > about the old row." We still get the same error when we try the DELETE
> > > again.
> >
> > Well, "REPLICA IDENTITY NOTHING" is the same as "has no replica identity".
> > So is "REPLICA IDENTITY DEFAULT" if there is no primary key, or
> > "REPLICA IDENTITY USING INDEX ..." if the index is dropped.
> >
> > See "pg_class": the column "relreplident" is not nullable.
>
> Right, I think the confusing point for us is that the docs for NOTHING
> ("Records no information about the old row") imply you can decide you
> don't have to record anything if you don't want to do so, but the
> publication feature is effectively overriding that and asserting that
> you can't make that choice.
>
Hi, I can see how the current docs could be interpreted in a way that
was not intended.
~~~
To emphasise the DEFAULT behaviour that Laurenze described, I felt
there could be another sentence about DEFAULT, the same as there is
already for the USING INDEX case.
BEFORE [1]
Records the old values of the columns of the primary key, if any. This
is the default for non-system tables.
SUGGESTION
Records the old values of the columns of the primary key, if any. This
is the default for non-system tables. If there is no primary key, the
behavior is the same as NOTHING.
~~~
If that is done, then would a publication docs tweak like the one
below clarify things sufficiently?
BEFORE [2]
If a table without a replica identity is added to a publication that
replicates UPDATE or DELETE operations then subsequent UPDATE or
DELETE operations will cause an error on the publisher.
SUGGESTION
If a table without a replica identity (or with replica identity
behavior equivalent to NOTHING) is added to a publication that
replicates UPDATE or DELETE operations then subsequent UPDATE or
DELETE operations will cause an error on the publisher.
======
[1] https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-REPLICA-IDENTITY
[2] https://www.postgresql.org/docs/current/logical-replication-publication.html
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING
@ 2024-02-08 00:12 James Coleman <[email protected]>
parent: Peter Smith <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: James Coleman @ 2024-02-08 00:12 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: Laurenz Albe <[email protected]>; pgsql-hackers
On Wed, Feb 7, 2024 at 6:04 PM Peter Smith <[email protected]> wrote:
>
> On Thu, Feb 8, 2024 at 9:04 AM James Coleman <[email protected]> wrote:
> >
> > On Wed, Feb 7, 2024 at 3:22 PM Laurenz Albe <[email protected]> wrote:
> > >
> > > On Wed, 2024-02-07 at 15:12 -0500, James Coleman wrote:
> > > > We recently noticed some behavior that seems reasonable but also
> > > > surprised our engineers based on the docs.
> > > >
> > > > If we have this setup:
> > > > create table items(i int);
> > > > insert into items(i) values (1);
> > > > create publication test_pub for all tables;
> > > >
> > > > Then when we:
> > > > delete from items where i = 1;
> > > >
> > > > we get:
> > > > ERROR: cannot delete from table "items" because it does not have a
> > > > replica identity and publishes deletes
> > > > HINT: To enable deleting from the table, set REPLICA IDENTITY using
> > > > ALTER TABLE.
> > > >
> > > > Fair enough. But if we do this:
> > > > alter table items replica identity nothing;
> > > >
> > > > because the docs [1] say that NOTHING means "Records no information
> > > > about the old row." We still get the same error when we try the DELETE
> > > > again.
> > >
> > > Well, "REPLICA IDENTITY NOTHING" is the same as "has no replica identity".
> > > So is "REPLICA IDENTITY DEFAULT" if there is no primary key, or
> > > "REPLICA IDENTITY USING INDEX ..." if the index is dropped.
> > >
> > > See "pg_class": the column "relreplident" is not nullable.
> >
> > Right, I think the confusing point for us is that the docs for NOTHING
> > ("Records no information about the old row") imply you can decide you
> > don't have to record anything if you don't want to do so, but the
> > publication feature is effectively overriding that and asserting that
> > you can't make that choice.
> >
>
> Hi, I can see how the current docs could be interpreted in a way that
> was not intended.
>
> ~~~
>
> To emphasise the DEFAULT behaviour that Laurenze described, I felt
> there could be another sentence about DEFAULT, the same as there is
> already for the USING INDEX case.
>
> BEFORE [1]
> Records the old values of the columns of the primary key, if any. This
> is the default for non-system tables.
>
> SUGGESTION
> Records the old values of the columns of the primary key, if any. This
> is the default for non-system tables. If there is no primary key, the
> behavior is the same as NOTHING.
>
> ~~~
>
> If that is done, then would a publication docs tweak like the one
> below clarify things sufficiently?
>
> BEFORE [2]
> If a table without a replica identity is added to a publication that
> replicates UPDATE or DELETE operations then subsequent UPDATE or
> DELETE operations will cause an error on the publisher.
>
> SUGGESTION
> If a table without a replica identity (or with replica identity
> behavior equivalent to NOTHING) is added to a publication that
> replicates UPDATE or DELETE operations then subsequent UPDATE or
> DELETE operations will cause an error on the publisher.
>
> ======
> [1] https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-REPLICA-IDENTITY
> [2] https://www.postgresql.org/docs/current/logical-replication-publication.html
>
> Kind Regards,
> Peter Smith.
> Fujitsu Australia
Thanks for looking at this!
Yes, both of those changes together would make this unambiguous (and,
I think, easier to mentally parse).
Thanks,
James Coleman
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING
@ 2024-02-08 02:40 Peter Smith <[email protected]>
parent: James Coleman <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Peter Smith @ 2024-02-08 02:40 UTC (permalink / raw)
To: James Coleman <[email protected]>; +Cc: Laurenz Albe <[email protected]>; pgsql-hackers
On Thu, Feb 8, 2024 at 11:12 AM James Coleman <[email protected]> wrote:
>
> On Wed, Feb 7, 2024 at 6:04 PM Peter Smith <[email protected]> wrote:
> >
> > On Thu, Feb 8, 2024 at 9:04 AM James Coleman <[email protected]> wrote:
> > >
> > > On Wed, Feb 7, 2024 at 3:22 PM Laurenz Albe <[email protected]> wrote:
> > > >
> > > > On Wed, 2024-02-07 at 15:12 -0500, James Coleman wrote:
> > > > > We recently noticed some behavior that seems reasonable but also
> > > > > surprised our engineers based on the docs.
> > > > >
> > > > > If we have this setup:
> > > > > create table items(i int);
> > > > > insert into items(i) values (1);
> > > > > create publication test_pub for all tables;
> > > > >
> > > > > Then when we:
> > > > > delete from items where i = 1;
> > > > >
> > > > > we get:
> > > > > ERROR: cannot delete from table "items" because it does not have a
> > > > > replica identity and publishes deletes
> > > > > HINT: To enable deleting from the table, set REPLICA IDENTITY using
> > > > > ALTER TABLE.
> > > > >
> > > > > Fair enough. But if we do this:
> > > > > alter table items replica identity nothing;
> > > > >
> > > > > because the docs [1] say that NOTHING means "Records no information
> > > > > about the old row." We still get the same error when we try the DELETE
> > > > > again.
> > > >
> > > > Well, "REPLICA IDENTITY NOTHING" is the same as "has no replica identity".
> > > > So is "REPLICA IDENTITY DEFAULT" if there is no primary key, or
> > > > "REPLICA IDENTITY USING INDEX ..." if the index is dropped.
> > > >
> > > > See "pg_class": the column "relreplident" is not nullable.
> > >
> > > Right, I think the confusing point for us is that the docs for NOTHING
> > > ("Records no information about the old row") imply you can decide you
> > > don't have to record anything if you don't want to do so, but the
> > > publication feature is effectively overriding that and asserting that
> > > you can't make that choice.
> > >
> >
> > Hi, I can see how the current docs could be interpreted in a way that
> > was not intended.
> >
> > ~~~
> >
> > To emphasise the DEFAULT behaviour that Laurenze described, I felt
> > there could be another sentence about DEFAULT, the same as there is
> > already for the USING INDEX case.
> >
> > BEFORE [1]
> > Records the old values of the columns of the primary key, if any. This
> > is the default for non-system tables.
> >
> > SUGGESTION
> > Records the old values of the columns of the primary key, if any. This
> > is the default for non-system tables. If there is no primary key, the
> > behavior is the same as NOTHING.
> >
> > ~~~
> >
> > If that is done, then would a publication docs tweak like the one
> > below clarify things sufficiently?
> >
> > BEFORE [2]
> > If a table without a replica identity is added to a publication that
> > replicates UPDATE or DELETE operations then subsequent UPDATE or
> > DELETE operations will cause an error on the publisher.
> >
> > SUGGESTION
> > If a table without a replica identity (or with replica identity
> > behavior equivalent to NOTHING) is added to a publication that
> > replicates UPDATE or DELETE operations then subsequent UPDATE or
> > DELETE operations will cause an error on the publisher.
> >
> > ======
> > [1] https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-REPLICA-IDENTITY
> > [2] https://www.postgresql.org/docs/current/logical-replication-publication.html
> >
> > Kind Regards,
> > Peter Smith.
> > Fujitsu Australia
>
> Thanks for looking at this!
>
> Yes, both of those changes together would make this unambiguous (and,
> I think, easier to mentally parse).
>
OK, here then is a patch to do like that.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
[application/octet-stream] v1-0001-replica-identity-clarifications.patch (1.9K, ../../CAHut+PuKND508Kc3BEacgrrSJSwOPzdOC1up-J_c_MbkE81Oyw@mail.gmail.com/2-v1-0001-replica-identity-clarifications.patch)
download | inline diff:
From 842fb97fe9662476d9f38836a16128c55d79e079 Mon Sep 17 00:00:00 2001
From: Peter Smith <[email protected]>
Date: Thu, 8 Feb 2024 13:33:51 +1100
Subject: [PATCH v1] replica identity clarifications
---
doc/src/sgml/logical-replication.sgml | 3 ++-
doc/src/sgml/ref/alter_table.sgml | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
index ec21306..44d6b2c 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -144,7 +144,8 @@
than <literal>FULL</literal> is set on the publisher side, a replica identity
comprising the same or fewer columns must also be set on the subscriber
side. See <xref linkend="sql-altertable-replica-identity"/> for details on
- how to set the replica identity. If a table without a replica identity is
+ how to set the replica identity. If a table without a replica identity
+ (or with replica identity behavior the same as <literal>NOTHING</literal>) is
added to a publication that replicates <command>UPDATE</command>
or <command>DELETE</command> operations then
subsequent <command>UPDATE</command> or <command>DELETE</command>
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 9670671..a844195 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -913,7 +913,8 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<listitem>
<para>
Records the old values of the columns of the primary key, if any.
- This is the default for non-system tables.
+ This is the default for non-system tables. If there is no primary key,
+ the behavior is the same as <literal>NOTHING</literal>.
</para>
</listitem>
</varlistentry>
--
1.8.3.1
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING
@ 2024-02-08 04:27 Laurenz Albe <[email protected]>
parent: Peter Smith <[email protected]>
0 siblings, 3 replies; 10+ messages in thread
From: Laurenz Albe @ 2024-02-08 04:27 UTC (permalink / raw)
To: Peter Smith <[email protected]>; James Coleman <[email protected]>; +Cc: pgsql-hackers
On Thu, 2024-02-08 at 13:40 +1100, Peter Smith wrote:
> - how to set the replica identity. If a table without a replica identity is
> + how to set the replica identity. If a table without a replica identity
> + (or with replica identity behavior the same as <literal>NOTHING</literal>) is
> added to a publication that replicates <command>UPDATE</command>
> or <command>DELETE</command> operations then
> subsequent <command>UPDATE</command> or <command>DELETE</command>
I had the impression that the root of the confusion was the perceived difference
between "REPLICA IDENTITY NOTHING" and "no replica identity", and that change
doesn't improve that.
How about:
If a table without a replica identity (explicitly set to <literal>NOTHING</literal>,
or set to a primary key or index that doesn't exist) is added ...
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING
@ 2024-02-08 09:46 Ashutosh Bapat <[email protected]>
parent: Laurenz Albe <[email protected]>
2 siblings, 0 replies; 10+ messages in thread
From: Ashutosh Bapat @ 2024-02-08 09:46 UTC (permalink / raw)
To: Laurenz Albe <[email protected]>; +Cc: Peter Smith <[email protected]>; James Coleman <[email protected]>; pgsql-hackers
On Thu, Feb 8, 2024 at 9:57 AM Laurenz Albe <[email protected]> wrote:
>
> On Thu, 2024-02-08 at 13:40 +1100, Peter Smith wrote:
> > - how to set the replica identity. If a table without a replica identity is
> > + how to set the replica identity. If a table without a replica identity
> > + (or with replica identity behavior the same as <literal>NOTHING</literal>) is
> > added to a publication that replicates <command>UPDATE</command>
> > or <command>DELETE</command> operations then
> > subsequent <command>UPDATE</command> or <command>DELETE</command>
>
> I had the impression that the root of the confusion was the perceived difference
> between "REPLICA IDENTITY NOTHING" and "no replica identity", and that change
> doesn't improve that.
>
> How about:
>
> If a table without a replica identity (explicitly set to <literal>NOTHING</literal>,
> or set to a primary key or index that doesn't exist) is added ...
Another possibility is just to improve the documentation of various
options as follows.
DEFAULT
If there is a primary key, record the old values of the columns of the
primary key. Otherwise it acts as NOTHING. This is the default for
non-system tables.
USING INDEX index_name
Records the old values of the columns covered by the named index, that
must be unique, not partial, not deferrable, and include only columns
marked NOT NULL. If this index is dropped, the behavior is the same as
NOTHING.
FULL
Records the old values of all columns in the row.
NOTHING
Records no information about the old row. This is equivalent to having
no replica identity. This is the default for system tables.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING
@ 2024-02-08 13:53 James Coleman <[email protected]>
parent: Laurenz Albe <[email protected]>
2 siblings, 0 replies; 10+ messages in thread
From: James Coleman @ 2024-02-08 13:53 UTC (permalink / raw)
To: Laurenz Albe <[email protected]>; +Cc: Peter Smith <[email protected]>; pgsql-hackers
On Wed, Feb 7, 2024 at 11:27 PM Laurenz Albe <[email protected]> wrote:
>
> On Thu, 2024-02-08 at 13:40 +1100, Peter Smith wrote:
> > - how to set the replica identity. If a table without a replica identity is
> > + how to set the replica identity. If a table without a replica identity
> > + (or with replica identity behavior the same as <literal>NOTHING</literal>) is
> > added to a publication that replicates <command>UPDATE</command>
> > or <command>DELETE</command> operations then
> > subsequent <command>UPDATE</command> or <command>DELETE</command>
>
> I had the impression that the root of the confusion was the perceived difference
> between "REPLICA IDENTITY NOTHING" and "no replica identity", and that change
> doesn't improve that.
>
> How about:
>
> If a table without a replica identity (explicitly set to <literal>NOTHING</literal>,
> or set to a primary key or index that doesn't exist) is added ...
I think that would work also. I was reading the initial suggestion as
"(or with replica identity behavior the same as..." as defining what
"without a replica identity" meant, which would avoid the confusion.
But your proposal is more explicit and more succinct, so I think it's
the better option of the two.
Regards,
James Coleman
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING
@ 2024-12-18 10:56 Amit Kapila <[email protected]>
parent: Laurenz Albe <[email protected]>
2 siblings, 0 replies; 10+ messages in thread
From: Amit Kapila @ 2024-12-18 10:56 UTC (permalink / raw)
To: Laurenz Albe <[email protected]>; +Cc: Peter Smith <[email protected]>; James Coleman <[email protected]>; pgsql-hackers
On Thu, Feb 8, 2024 at 9:57 AM Laurenz Albe <[email protected]> wrote:
>
> On Thu, 2024-02-08 at 13:40 +1100, Peter Smith wrote:
> > - how to set the replica identity. If a table without a replica identity is
> > + how to set the replica identity. If a table without a replica identity
> > + (or with replica identity behavior the same as <literal>NOTHING</literal>) is
> > added to a publication that replicates <command>UPDATE</command>
> > or <command>DELETE</command> operations then
> > subsequent <command>UPDATE</command> or <command>DELETE</command>
>
> I had the impression that the root of the confusion was the perceived difference
> between "REPLICA IDENTITY NOTHING" and "no replica identity", and that change
> doesn't improve that.
>
> How about:
>
> If a table without a replica identity (explicitly set to <literal>NOTHING</literal>,
> or set to a primary key or index that doesn't exist) is added ...
>
Is it correct to say "set to a primary key or index that doesn't
exist"? Because when it is set to the primary key then it should work.
I think Peter's proposal along with Ashutosh's proposal is the simpler
approach to clarify things in this area but I am fine if others find
some other way of updating docs better.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-12-18 10:56 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-12-11 12:54 [PATCH v25 4/5] Add ALTER INDEX ... ALTER COLLATION ... REFRESH VERSION. Julien Rouhaud <[email protected]>
2024-02-07 20:22 Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING Laurenz Albe <[email protected]>
2024-02-07 22:03 ` Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING James Coleman <[email protected]>
2024-02-07 23:04 ` Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING Peter Smith <[email protected]>
2024-02-08 00:12 ` Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING James Coleman <[email protected]>
2024-02-08 02:40 ` Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING Peter Smith <[email protected]>
2024-02-08 04:27 ` Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING Laurenz Albe <[email protected]>
2024-02-08 09:46 ` Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING Ashutosh Bapat <[email protected]>
2024-02-08 13:53 ` Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING James Coleman <[email protected]>
2024-12-18 10:56 ` Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING Amit Kapila <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox