public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v1 1/8] Add jsonpath 'pg' modifier for enabling extensions
17+ messages / 7 participants
[nested] [flat]
* [PATCH v1 1/8] Add jsonpath 'pg' modifier for enabling extensions
@ 2019-06-27 16:58 Nikita Glukhov <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Nikita Glukhov @ 2019-06-27 16:58 UTC (permalink / raw)
---
doc/src/sgml/func.sgml | 29 ++++++++++++++++++
src/backend/utils/adt/jsonpath.c | 54 ++++++++++++++++++++++++++--------
src/backend/utils/adt/jsonpath_exec.c | 4 +++
src/backend/utils/adt/jsonpath_gram.y | 17 +++++++----
src/backend/utils/adt/jsonpath_scan.l | 1 +
src/include/utils/jsonpath.h | 9 ++++--
src/test/regress/expected/jsonpath.out | 18 ++++++++++++
src/test/regress/sql/jsonpath.sql | 3 ++
src/tools/pgindent/typedefs.list | 1 +
9 files changed, 116 insertions(+), 20 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 28035f1..d344b95 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12601,6 +12601,35 @@ table2-mapping
</sect3>
+ <sect3 id="pg-extensions">
+ <title>Extensions</title>
+ <para>
+ <productname>PostgreSQL</productname> has some extensions to the SQL/JSON
+ Path standard. These syntax extensions that can enabled by the specifying
+ additional <literal>pg</literal> modifier before the
+ <literal>strict</literal>/<literal>lax</literal> flags.
+ <xref linkend="functions-jsonpath-extensions"/> shows these
+ extensions with examples.
+ </para>
+
+ <table id="functions-jsonpath-extensions">
+ <title><type>jsonpath</type> Syntax Extensions</title>
+ <tgroup cols="5">
+ <thead>
+ <row>
+ <entry>Name</entry>
+ <entry>Description</entry>
+ <entry>Example JSON</entry>
+ <entry>Example JSON path</entry>
+ <entry>Result</entry>
+ </row>
+ </thead>
+ <tbody>
+ </tbody>
+ </tgroup>
+ </table>
+ </sect3>
+
<sect3 id="jsonpath-regular-expressions">
<title>Regular Expressions</title>
diff --git a/src/backend/utils/adt/jsonpath.c b/src/backend/utils/adt/jsonpath.c
index 3c0dc38..17c09a7 100644
--- a/src/backend/utils/adt/jsonpath.c
+++ b/src/backend/utils/adt/jsonpath.c
@@ -72,11 +72,20 @@
#include "utils/jsonpath.h"
+/* Context for jsonpath encoding. */
+typedef struct JsonPathEncodingContext
+{
+ StringInfo buf; /* output buffer */
+ bool ext; /* PG extensions are enabled? */
+} JsonPathEncodingContext;
+
static Datum jsonPathFromCstring(char *in, int len);
static char *jsonPathToCstring(StringInfo out, JsonPath *in,
int estimated_len);
-static int flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem *item,
- int nestingLevel, bool insideArraySubscript);
+static int flattenJsonPathParseItem(JsonPathEncodingContext *cxt,
+ JsonPathParseItem *item,
+ int nestingLevel,
+ bool insideArraySubscript);
static void alignStringInfoInt(StringInfo buf);
static int32 reserveSpaceForItemPointer(StringInfo buf);
static void printJsonPathItem(StringInfo buf, JsonPathItem *v, bool inKey,
@@ -167,6 +176,7 @@ jsonpath_send(PG_FUNCTION_ARGS)
static Datum
jsonPathFromCstring(char *in, int len)
{
+ JsonPathEncodingContext cxt;
JsonPathParseResult *jsonpath = parsejsonpath(in, len);
JsonPath *res;
StringInfoData buf;
@@ -182,13 +192,18 @@ jsonPathFromCstring(char *in, int len)
errmsg("invalid input syntax for type %s: \"%s\"", "jsonpath",
in)));
- flattenJsonPathParseItem(&buf, jsonpath->expr, 0, false);
+ cxt.buf = &buf;
+ cxt.ext = jsonpath->ext;
+
+ flattenJsonPathParseItem(&cxt, jsonpath->expr, 0, false);
res = (JsonPath *) buf.data;
SET_VARSIZE(res, buf.len);
res->header = JSONPATH_VERSION;
if (jsonpath->lax)
res->header |= JSONPATH_LAX;
+ if (jsonpath->ext)
+ res->header |= JSONPATH_EXT;
PG_RETURN_JSONPATH_P(res);
}
@@ -212,6 +227,8 @@ jsonPathToCstring(StringInfo out, JsonPath *in, int estimated_len)
}
enlargeStringInfo(out, estimated_len);
+ if (in->header & JSONPATH_EXT)
+ appendBinaryStringInfo(out, "pg ", 3);
if (!(in->header & JSONPATH_LAX))
appendBinaryStringInfo(out, "strict ", 7);
@@ -221,14 +238,27 @@ jsonPathToCstring(StringInfo out, JsonPath *in, int estimated_len)
return out->data;
}
+static void
+checkJsonPathExtensionsEnabled(JsonPathEncodingContext *cxt,
+ JsonPathItemType type)
+{
+ if (!cxt->ext)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("%s contains extended operators that were not enabled", "jsonpath"),
+ errhint("use \"%s\" modifier at the start of %s string to enable extensions",
+ "pg", "jsonpath")));
+}
+
/*
* Recursive function converting given jsonpath parse item and all its
* children into a binary representation.
*/
static int
-flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem *item,
+flattenJsonPathParseItem(JsonPathEncodingContext *cxt, JsonPathParseItem *item,
int nestingLevel, bool insideArraySubscript)
{
+ StringInfo buf = cxt->buf;
/* position from beginning of jsonpath data */
int32 pos = buf->len - JSONPATH_HDRSZ;
int32 chld;
@@ -296,13 +326,13 @@ flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem *item,
int32 right = reserveSpaceForItemPointer(buf);
chld = !item->value.args.left ? pos :
- flattenJsonPathParseItem(buf, item->value.args.left,
+ flattenJsonPathParseItem(cxt, item->value.args.left,
nestingLevel + argNestingLevel,
insideArraySubscript);
*(int32 *) (buf->data + left) = chld - pos;
chld = !item->value.args.right ? pos :
- flattenJsonPathParseItem(buf, item->value.args.right,
+ flattenJsonPathParseItem(cxt, item->value.args.right,
nestingLevel + argNestingLevel,
insideArraySubscript);
*(int32 *) (buf->data + right) = chld - pos;
@@ -323,7 +353,7 @@ flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem *item,
item->value.like_regex.patternlen);
appendStringInfoChar(buf, '\0');
- chld = flattenJsonPathParseItem(buf, item->value.like_regex.expr,
+ chld = flattenJsonPathParseItem(cxt, item->value.like_regex.expr,
nestingLevel,
insideArraySubscript);
*(int32 *) (buf->data + offs) = chld - pos;
@@ -342,7 +372,7 @@ flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem *item,
int32 arg = reserveSpaceForItemPointer(buf);
chld = !item->value.arg ? pos :
- flattenJsonPathParseItem(buf, item->value.arg,
+ flattenJsonPathParseItem(cxt, item->value.arg,
nestingLevel + argNestingLevel,
insideArraySubscript);
*(int32 *) (buf->data + arg) = chld - pos;
@@ -384,12 +414,12 @@ flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem *item,
int32 *ppos;
int32 topos;
int32 frompos =
- flattenJsonPathParseItem(buf,
+ flattenJsonPathParseItem(cxt,
item->value.array.elems[i].from,
nestingLevel, true) - pos;
if (item->value.array.elems[i].to)
- topos = flattenJsonPathParseItem(buf,
+ topos = flattenJsonPathParseItem(cxt,
item->value.array.elems[i].to,
nestingLevel, true) - pos;
else
@@ -424,7 +454,7 @@ flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem *item,
if (item->next)
{
- chld = flattenJsonPathParseItem(buf, item->next, nestingLevel,
+ chld = flattenJsonPathParseItem(cxt, item->next, nestingLevel,
insideArraySubscript) - pos;
*(int32 *) (buf->data + next) = chld;
}
@@ -832,7 +862,7 @@ operationPriority(JsonPathItemType op)
void
jspInit(JsonPathItem *v, JsonPath *js)
{
- Assert((js->header & ~JSONPATH_LAX) == JSONPATH_VERSION);
+ Assert((js->header & JSONPATH_VERSION_MASK) == JSONPATH_VERSION);
jspInitByBuffer(v, js->data, 0);
}
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index b6fdd47..9f13f4b 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -101,6 +101,8 @@ typedef struct JsonPathExecContext
int innermostArraySize; /* for LAST array index evaluation */
bool laxMode; /* true for "lax" mode, false for "strict"
* mode */
+ bool useExtensions; /* use PostgreSQL-specific extensions?
+ * (enabled by 'pg' modifier in jsonpath) */
bool ignoreStructuralErrors; /* with "true" structural errors such
* as absence of required json item or
* unexpected json item type are
@@ -157,6 +159,7 @@ typedef struct JsonValueListIterator
#define jspAutoWrap(cxt) ((cxt)->laxMode)
#define jspIgnoreStructuralErrors(cxt) ((cxt)->ignoreStructuralErrors)
#define jspThrowErrors(cxt) ((cxt)->throwErrors)
+#define jspUseExtensions(cxt) ((cxt)->useExtensions)
/* Convenience macro: return or throw error depending on context */
#define RETURN_ERROR(throw_error) \
@@ -559,6 +562,7 @@ executeJsonPath(JsonPath *path, Jsonb *vars, Jsonb *json, bool throwErrors,
cxt.vars = vars;
cxt.laxMode = (path->header & JSONPATH_LAX) != 0;
+ cxt.useExtensions = (path->header & JSONPATH_EXT) != 0;
cxt.ignoreStructuralErrors = cxt.laxMode;
cxt.root = &jbv;
cxt.current = &jbv;
diff --git a/src/backend/utils/adt/jsonpath_gram.y b/src/backend/utils/adt/jsonpath_gram.y
index f87db8c..426dbb1 100644
--- a/src/backend/utils/adt/jsonpath_gram.y
+++ b/src/backend/utils/adt/jsonpath_gram.y
@@ -88,7 +88,7 @@ static JsonPathParseItem *makeItemLikeRegex(JsonPathParseItem *expr,
int integer;
}
-%token <str> TO_P NULL_P TRUE_P FALSE_P IS_P UNKNOWN_P EXISTS_P
+%token <str> TO_P NULL_P TRUE_P FALSE_P IS_P UNKNOWN_P EXISTS_P PG_P
%token <str> IDENT_P STRING_P NUMERIC_P INT_P VARIABLE_P
%token <str> OR_P AND_P NOT_P
%token <str> LESS_P LESSEQUAL_P EQUAL_P NOTEQUAL_P GREATEREQUAL_P GREATER_P
@@ -109,7 +109,7 @@ static JsonPathParseItem *makeItemLikeRegex(JsonPathParseItem *expr,
%type <optype> comp_op method
-%type <boolean> mode
+%type <boolean> mode pg_opt
%type <str> key_name
@@ -127,10 +127,11 @@ static JsonPathParseItem *makeItemLikeRegex(JsonPathParseItem *expr,
%%
result:
- mode expr_or_predicate {
+ pg_opt mode expr_or_predicate {
*result = palloc(sizeof(JsonPathParseResult));
- (*result)->expr = $2;
- (*result)->lax = $1;
+ (*result)->expr = $3;
+ (*result)->lax = $2;
+ (*result)->ext = $1;
}
| /* EMPTY */ { *result = NULL; }
;
@@ -140,6 +141,11 @@ expr_or_predicate:
| predicate { $$ = $1; }
;
+pg_opt:
+ PG_P { $$ = true; }
+ | /* EMPTY */ { $$ = false; }
+ ;
+
mode:
STRICT_P { $$ = false; }
| LAX_P { $$ = true; }
@@ -292,6 +298,7 @@ key_name:
| WITH_P
| LIKE_REGEX_P
| FLAG_P
+ | PG_P
;
method:
diff --git a/src/backend/utils/adt/jsonpath_scan.l b/src/backend/utils/adt/jsonpath_scan.l
index 70681b7..c8bce27 100644
--- a/src/backend/utils/adt/jsonpath_scan.l
+++ b/src/backend/utils/adt/jsonpath_scan.l
@@ -305,6 +305,7 @@ typedef struct JsonPathKeyword
*/
static const JsonPathKeyword keywords[] = {
{ 2, false, IS_P, "is"},
+ { 2, false, PG_P, "pg"},
{ 2, false, TO_P, "to"},
{ 3, false, ABS_P, "abs"},
{ 3, false, LAX_P, "lax"},
diff --git a/src/include/utils/jsonpath.h b/src/include/utils/jsonpath.h
index 4ef0880..d56175f 100644
--- a/src/include/utils/jsonpath.h
+++ b/src/include/utils/jsonpath.h
@@ -25,8 +25,10 @@ typedef struct
char data[FLEXIBLE_ARRAY_MEMBER];
} JsonPath;
-#define JSONPATH_VERSION (0x01)
-#define JSONPATH_LAX (0x80000000)
+#define JSONPATH_VERSION 0x01
+#define JSONPATH_LAX 0x80000000 /* lax/strict mode */
+#define JSONPATH_EXT 0x40000000 /* PG extensions */
+#define JSONPATH_VERSION_MASK (~(JSONPATH_LAX | JSONPATH_EXT))
#define JSONPATH_HDRSZ (offsetof(JsonPath, data))
#define DatumGetJsonPathP(d) ((JsonPath *) DatumGetPointer(PG_DETOAST_DATUM(d)))
@@ -241,7 +243,8 @@ struct JsonPathParseItem
typedef struct JsonPathParseResult
{
JsonPathParseItem *expr;
- bool lax;
+ bool lax; /* lax/strict mode */
+ bool ext; /* PostgreSQL extensions */
} JsonPathParseResult;
extern JsonPathParseResult *parsejsonpath(const char *str, int len);
diff --git a/src/test/regress/expected/jsonpath.out b/src/test/regress/expected/jsonpath.out
index e399fa9..52b36a8 100644
--- a/src/test/regress/expected/jsonpath.out
+++ b/src/test/regress/expected/jsonpath.out
@@ -21,6 +21,24 @@ select 'lax $'::jsonpath;
$
(1 row)
+select 'pg $'::jsonpath;
+ jsonpath
+----------
+ pg $
+(1 row)
+
+select 'pg strict $'::jsonpath;
+ jsonpath
+-------------
+ pg strict $
+(1 row)
+
+select 'pg lax $'::jsonpath;
+ jsonpath
+----------
+ pg $
+(1 row)
+
select '$.a'::jsonpath;
jsonpath
----------
diff --git a/src/test/regress/sql/jsonpath.sql b/src/test/regress/sql/jsonpath.sql
index 17ab775..315e42f 100644
--- a/src/test/regress/sql/jsonpath.sql
+++ b/src/test/regress/sql/jsonpath.sql
@@ -4,6 +4,9 @@ select ''::jsonpath;
select '$'::jsonpath;
select 'strict $'::jsonpath;
select 'lax $'::jsonpath;
+select 'pg $'::jsonpath;
+select 'pg strict $'::jsonpath;
+select 'pg lax $'::jsonpath;
select '$.a'::jsonpath;
select '$.a.v'::jsonpath;
select '$.a.*'::jsonpath;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index e216de9..d4148e1 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1130,6 +1130,7 @@ JsonLikeRegexContext
JsonParseContext
JsonPath
JsonPathBool
+JsonPathEncodingContext
JsonPathExecContext
JsonPathExecResult
JsonPathGinAddPathItemFunc
--
2.7.4
--------------574DCE16CAD28E71E2083DCD
Content-Type: text/x-patch;
name="v1-0002-Add-raw-jbvArray-and-jbvObject-support-to-jsonpat.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="v1-0002-Add-raw-jbvArray-and-jbvObject-support-to-jsonpat.pa";
filename*1="tch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* Confused comment about drop replica identity index
@ 2021-12-14 12:38 [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: [email protected] @ 2021-12-14 12:38 UTC (permalink / raw)
To: [email protected] <[email protected]>
Hi hackers,
When I doing development based by PG, I found the following comment have a
little problem in file src/include/catalog/pg_class.h.
/*
* an explicitly chosen candidate key's columns are used as replica identity.
* Note this will still be set if the index has been dropped; in that case it
* has the same meaning as 'd'.
*/
#define REPLICA_IDENTITY_INDEX 'i'
The last sentence makes me a little confused :
[......in that case it as the same meaning as 'd'.]
Now, pg-doc didn't have a clear style to describe this.
But if I drop relation's replica identity index like the comment, the action
is not as same as default.
For example:
Execute the following SQL:
create table tbl (col1 int primary key, col2 int not null);
create unique INDEX ON tbl(col2);
alter table tbl replica identity using INDEX tbl_col2_idx;
drop index tbl_col2_idx;
create publication pub for table tbl;
delete from tbl;
Actual result:
ERROR: cannot delete from table "tbl" because it does not have a replica identity and publishes deletes
HINT: To enable deleting from the table, set REPLICA IDENTITY using ALTER TABLE.
Expected result in comment:
DELETE 0
I found that in the function CheckCmdReplicaIdentity, the operation described
in the comment is not considered,
When relation's replica identity index is found to be InvalidOid, an error is
reported.
Are the comment here not accurate enough?
Or we need to adjust the code according to the comments?
Regards,
Wang wei
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
@ 2021-12-14 13:40 ` Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Ashutosh Bapat @ 2021-12-14 13:40 UTC (permalink / raw)
To: [email protected] <[email protected]>; +Cc: [email protected] <[email protected]>
On Tue, Dec 14, 2021 at 6:08 PM [email protected]
<[email protected]> wrote:
>
> Hi hackers,
>
> When I doing development based by PG, I found the following comment have a
> little problem in file src/include/catalog/pg_class.h.
>
> /*
> * an explicitly chosen candidate key's columns are used as replica identity.
> * Note this will still be set if the index has been dropped; in that case it
> * has the same meaning as 'd'.
> */
> #define REPLICA_IDENTITY_INDEX 'i'
>
> The last sentence makes me a little confused :
> [......in that case it as the same meaning as 'd'.]
>
> Now, pg-doc didn't have a clear style to describe this.
>
>
> But if I drop relation's replica identity index like the comment, the action
> is not as same as default.
>
> For example:
> Execute the following SQL:
> create table tbl (col1 int primary key, col2 int not null);
> create unique INDEX ON tbl(col2);
> alter table tbl replica identity using INDEX tbl_col2_idx;
> drop index tbl_col2_idx;
> create publication pub for table tbl;
> delete from tbl;
>
> Actual result:
> ERROR: cannot delete from table "tbl" because it does not have a replica identity and publishes deletes
> HINT: To enable deleting from the table, set REPLICA IDENTITY using ALTER TABLE.
I think I see where's the confusion. The table has a primary key and
so when the replica identity index is dropped, per the comment in
code, you expect that primary key will be used as replica identity
since that's what 'd' or default means.
>
> Expected result in comment:
> DELETE 0
>
>
> I found that in the function CheckCmdReplicaIdentity, the operation described
> in the comment is not considered,
> When relation's replica identity index is found to be InvalidOid, an error is
> reported.
This code in RelationGetIndexList() is not according to that comment.
if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex))
relation->rd_replidindex = pkeyIndex;
else if (replident == REPLICA_IDENTITY_INDEX && OidIsValid(candidateIndex))
relation->rd_replidindex = candidateIndex;
else
relation->rd_replidindex = InvalidOid;
>
> Are the comment here not accurate enough?
> Or we need to adjust the code according to the comments?
>
Comment in code is one thing, but I think PG documentation is not
covering the use case you tried. What happens when a replica identity
index is dropped has not been covered either in ALTER TABLE
https://www.postgresql.org/docs/13/sql-altertable.html or DROP INDEX
https://www.postgresql.org/docs/14/sql-dropindex.html documentation.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
@ 2021-12-15 03:24 ` Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-16 18:08 ` Re: Confused comment about drop replica identity index Alvaro Herrera <[email protected]>
0 siblings, 2 replies; 17+ messages in thread
From: Michael Paquier @ 2021-12-15 03:24 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>
On Tue, Dec 14, 2021 at 07:10:49PM +0530, Ashutosh Bapat wrote:
> This code in RelationGetIndexList() is not according to that comment.
>
> if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex))
> relation->rd_replidindex = pkeyIndex;
> else if (replident == REPLICA_IDENTITY_INDEX && OidIsValid(candidateIndex))
> relation->rd_replidindex = candidateIndex;
> else
> relation->rd_replidindex = InvalidOid;
Yeah, the comment is wrong. If the index of a REPLICA_IDENTITY_INDEX
is dropped, I recall that the behavior is the same as
REPLICA_IDENTITY_NOTHING.
> Comment in code is one thing, but I think PG documentation is not
> covering the use case you tried. What happens when a replica identity
> index is dropped has not been covered either in ALTER TABLE
> https://www.postgresql.org/docs/13/sql-altertable.html or DROP INDEX
> https://www.postgresql.org/docs/14/sql-dropindex.html documentation.
Not sure about the DROP INDEX page, but I'd be fine with mentioning
that in the ALTER TABLE page in the paragraph related to REPLICA
IDENTITY. While on it, I would be tempted to switch this stuff to use
a list of <variablelist> for all the option values. That would be
much easier to read.
[ ... thinks a bit ... ]
FWIW, this brings back some memories, as of this thread:
https://www.postgresql.org/message-id/[email protected]
See also commit fe7fd4e from August 2020, where some tests have been
added. I recall seeing this incorrect comment from last year's
thread and it may have been mentioned in one of the surrounding
threads.. Maybe I just let it go back then. I don't know.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../Yblf%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* RE: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
@ 2021-12-15 09:18 ` [email protected] <[email protected]>
2021-12-15 22:39 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
1 sibling, 1 reply; 17+ messages in thread
From: [email protected] @ 2021-12-15 09:18 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: [email protected] <[email protected]>
On Tue, Dec 15, 2021 at 11:25AM, Michael Paquier wrote:
> Yeah, the comment is wrong. If the index of a REPLICA_IDENTITY_INDEX is
> dropped, I recall that the behavior is the same as REPLICA_IDENTITY_NOTHING.
Thank you for your response.
I agreed that the comment is wrong.
> Not sure about the DROP INDEX page, but I'd be fine with mentioning that in the
> ALTER TABLE page in the paragraph related to REPLICA IDENTITY. While on it, I
> would be tempted to switch this stuff to use a list of <variablelist> for all the option
> values. That would be much easier to read.
Yeah, if we can add some details to pg-doc and code comments, I think it will
be more friendly to PG users and developers.
Regards,
Wang wei
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
@ 2021-12-15 22:39 ` Michael Paquier <[email protected]>
2021-12-16 02:27 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Michael Paquier @ 2021-12-15 22:39 UTC (permalink / raw)
To: [email protected] <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Wed, Dec 15, 2021 at 09:18:26AM +0000, [email protected] wrote:
> Yeah, if we can add some details to pg-doc and code comments, I think it will
> be more friendly to PG users and developers.
Would you like to write a patch to address all that?
Thanks,
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* RE: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-15 22:39 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
@ 2021-12-16 02:27 ` [email protected] <[email protected]>
2021-12-20 03:46 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: [email protected] @ 2021-12-16 02:27 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Tue, Dec 16, 2021 at 06:40AM, Michael Paquier wrote:
> Would you like to write a patch to address all that?
OK, I will push it soon.
Regards,
Wang wei
^ permalink raw reply [nested|flat] 17+ messages in thread
* RE: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-15 22:39 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-16 02:27 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
@ 2021-12-20 03:46 ` [email protected] <[email protected]>
2021-12-20 11:11 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: [email protected] @ 2021-12-20 03:46 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Tue, Dec 16, 2021 at 10:27AM, Michael Paquier wrote:
> On Tue, Dec 16, 2021 at 06:40AM, Michael Paquier wrote:
> > Would you like to write a patch to address all that?
>
> OK, I will push it soon.
Here is a patch to correct wrong comment about REPLICA_IDENTITY_INDEX, And improve the pg-doc.
Regards,
Wang wei
Attachments:
[application/octet-stream] Correct-wrong-comment-about-REPLICA_IDENTITY_INDEX.patch (3.9K, ../../OS3PR01MB62750A0A1A282E9E0148172B9E7B9@OS3PR01MB6275.jpnprd01.prod.outlook.com/2-Correct-wrong-comment-about-REPLICA_IDENTITY_INDEX.patch)
download | inline diff:
From a77650dda3c724b7ad89926874b8e5f29c80f327 Mon Sep 17 00:00:00 2001
From: wangw <[email protected]>
Date: Fri, 17 Dec 2021 04:30:32 +0800
Subject: [PATCH] Correct wrong comment about REPLICA_IDENTITY_INDEX.
The comment about REPLICA_IDENTITY_INDEX is wrong.
When drop relation's replica identity index like the comment, the action is not
as same as DEFAULT. Just like NOTHING.
So correct this comment. And add this use case to PG-doc.
By the way, improve PG-doc to make it easier for users to read.
---
doc/src/sgml/ref/alter_table.sgml | 50 ++++++++++++++++++++++++++-----
src/include/catalog/pg_class.h | 2 +-
2 files changed, 43 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 8f14e4a5c4..1081d8d7bd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -872,16 +872,50 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<para>
This form changes the information which is written to the write-ahead log
to identify rows which are updated or deleted. This option has no effect
- except when logical replication is in use. <literal>DEFAULT</literal>
- (the default for non-system tables) records the
- old values of the columns of the primary key, if any. <literal>USING INDEX</literal>
- records the old values of the columns covered by the named index, which
- must be unique, not partial, not deferrable, and include only columns marked
- <literal>NOT NULL</literal>. <literal>FULL</literal> records the old values of all columns
- in the row. <literal>NOTHING</literal> records no information about the old row.
- (This is the default for system tables.)
+ except when logical replication is in use.
In all cases, no old values are logged unless at least one of the columns
that would be logged differs between the old and new versions of the row.
+ <variablelist>
+ <varlistentry>
+ <term><literal>DEFAULT</literal></term>
+ <listitem>
+ <para>
+ The default for non-system tables. Records the old values of the columns
+ of the primary key, if any. The default for non-system tables.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>USING INDEX index_name</literal></term>
+ <listitem>
+ <para>
+ Records the old values of the columns covered by the named index, which
+ must be unique, not partial,not deferrable, and include only columns
+ marked <literal>NOT NULL</literal>. If replica identity index is dropped,
+ the behavior is the same as <literal>NOTHING</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>FULL</literal></term>
+ <listitem>
+ <para>
+ Records the old values of all columns in the row.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>NOTHING</literal></term>
+ <listitem>
+ <para>
+ Records no information about the old row.(This is the default for system tables.)
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
</para>
</listitem>
</varlistentry>
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 93338d267c..b46299048e 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -182,7 +182,7 @@ DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, ClassTblspcRelfilenodeInd
/*
* an explicitly chosen candidate key's columns are used as replica identity.
* Note this will still be set if the index has been dropped; in that case it
- * has the same meaning as 'd'.
+ * has the same meaning as 'n'.
*/
#define REPLICA_IDENTITY_INDEX 'i'
--
2.27.0
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-15 22:39 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-16 02:27 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 03:46 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
@ 2021-12-20 11:11 ` Michael Paquier <[email protected]>
2021-12-20 14:57 ` Re: Confused comment about drop replica identity index Euler Taveira <[email protected]>
2021-12-21 01:31 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
0 siblings, 2 replies; 17+ messages in thread
From: Michael Paquier @ 2021-12-20 11:11 UTC (permalink / raw)
To: [email protected] <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Mon, Dec 20, 2021 at 03:46:13AM +0000, [email protected] wrote:
> Here is a patch to correct wrong comment about
> REPLICA_IDENTITY_INDEX, And improve the pg-doc.
That's mostly fine. I have made some adjustments as per the
attached.
+ The default for non-system tables. Records the old values of the columns
+ of the primary key, if any. The default for non-system tables.
The same sentence is repeated twice.
+ Records no information about the old row.(This is the
default for system tables.)
For consistency with the rest, this could drop the parenthesis for the
second sentence.
+ <term><literal>USING INDEX index_name</literal></term>
This should use <replaceable> as markup for index_name.
Pondering more about this thread, I don't think we should change the
existing behavior in the back-branches, but I don't have any arguments
about doing such changes on HEAD to help the features being worked
on, either. So I'd like to apply and back-patch the attached, as a
first step, to fix the inconsistency.
--
Michael
Attachments:
[text/x-diff] v2-0001-Correct-comment-and-documentation-about-REPLICA_I.patch (3.6K, ../../[email protected]/2-v2-0001-Correct-comment-and-documentation-about-REPLICA_I.patch)
download | inline diff:
From 05fe07224a5f31f3d28da088f302772724c00dd4 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Mon, 20 Dec 2021 20:09:59 +0900
Subject: [PATCH v2] Correct comment and documentation about
REPLICA_IDENTITY_INDEX
---
src/include/catalog/pg_class.h | 2 +-
doc/src/sgml/ref/alter_table.sgml | 51 ++++++++++++++++++++++++++-----
2 files changed, 44 insertions(+), 9 deletions(-)
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 93338d267c..b46299048e 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -182,7 +182,7 @@ DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, ClassTblspcRelfilenodeInd
/*
* an explicitly chosen candidate key's columns are used as replica identity.
* Note this will still be set if the index has been dropped; in that case it
- * has the same meaning as 'd'.
+ * has the same meaning as 'n'.
*/
#define REPLICA_IDENTITY_INDEX 'i'
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 8f14e4a5c4..a76e2e7322 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -872,16 +872,51 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<para>
This form changes the information which is written to the write-ahead log
to identify rows which are updated or deleted. This option has no effect
- except when logical replication is in use. <literal>DEFAULT</literal>
- (the default for non-system tables) records the
- old values of the columns of the primary key, if any. <literal>USING INDEX</literal>
- records the old values of the columns covered by the named index, which
- must be unique, not partial, not deferrable, and include only columns marked
- <literal>NOT NULL</literal>. <literal>FULL</literal> records the old values of all columns
- in the row. <literal>NOTHING</literal> records no information about the old row.
- (This is the default for system tables.)
+ except when logical replication is in use.
In all cases, no old values are logged unless at least one of the columns
that would be logged differs between the old and new versions of the row.
+ <variablelist>
+ <varlistentry>
+ <term><literal>DEFAULT</literal></term>
+ <listitem>
+ <para>
+ Records the old values of the columns of the primary key, if any.
+ This is the default for non-system tables.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>USING INDEX <replaceable class="parameter">index_name</replaceable></literal></term>
+ <listitem>
+ <para>
+ 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 <literal>NOT NULL</literal>. If this index is
+ dropped, the behavior is the same as <literal>NOTHING</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>FULL</literal></term>
+ <listitem>
+ <para>
+ Records the old values of all columns in the row.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>NOTHING</literal></term>
+ <listitem>
+ <para>
+ Records no information about the old row. This is the default for
+ system tables.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
</para>
</listitem>
</varlistentry>
--
2.34.1
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-15 22:39 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-16 02:27 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 03:46 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 11:11 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
@ 2021-12-20 14:57 ` Euler Taveira <[email protected]>
2021-12-21 00:46 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-22 07:40 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
1 sibling, 2 replies; 17+ messages in thread
From: Euler Taveira @ 2021-12-20 14:57 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; [email protected] <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Mon, Dec 20, 2021, at 8:11 AM, Michael Paquier wrote:
> On Mon, Dec 20, 2021 at 03:46:13AM +0000, [email protected] wrote:
> > Here is a patch to correct wrong comment about
> > REPLICA_IDENTITY_INDEX, And improve the pg-doc.
>
> That's mostly fine. I have made some adjustments as per the
> attached.
Your patch looks good to me.
> Pondering more about this thread, I don't think we should change the
> existing behavior in the back-branches, but I don't have any arguments
> about doing such changes on HEAD to help the features being worked
> on, either. So I'd like to apply and back-patch the attached, as a
> first step, to fix the inconsistency.
>
What do you think about the attached patch? It forbids the DROP INDEX. We might
add a detail message but I didn't in this patch.
--
Euler Taveira
EDB https://www.enterprisedb.com/
Attachments:
[text/x-patch] v1-0001-Disallow-dropping-an-index-that-is-used-by-replic.patch (3.7K, ../../[email protected]/3-v1-0001-Disallow-dropping-an-index-that-is-used-by-replic.patch)
download | inline diff:
From 2ba00335746cf8348e019582a253721047f1a0ac Mon Sep 17 00:00:00 2001
From: Euler Taveira <[email protected]>
Date: Thu, 16 Dec 2021 16:54:47 -0300
Subject: [PATCH v1] Disallow dropping an index that is used by replica
identity
Drop an index that is used by a replica identity can break the
replication for UPDATE and DELETE operations. This may be undesirable
from the usability standpoint. It introduces a behaviour change.
---
src/backend/commands/tablecmds.c | 14 +++++++++++++-
src/test/regress/expected/replica_identity.out | 3 +++
src/test/regress/sql/replica_identity.sql | 3 +++
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bf42587e38..727ad6625b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1530,15 +1530,21 @@ RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
rel->relname);
/*
+ * An index used by a replica identity cannot be dropped because some
+ * operations (UPDATE and DELETE) relies on the index properties
+ * (uniqueness and NOT NULL) to record old values. These old values are
+ * essential for identifying the row on the subscriber.
+ *
* Check the case of a system index that might have been invalidated by a
* failed concurrent process and allow its drop. For the time being, this
* only concerns indexes of toast relations that became invalid during a
* REINDEX CONCURRENTLY process.
*/
- if (IsSystemClass(relOid, classform) && relkind == RELKIND_INDEX)
+ if (relkind == RELKIND_INDEX)
{
HeapTuple locTuple;
Form_pg_index indexform;
+ bool indisreplident;
bool indisvalid;
locTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(relOid));
@@ -1550,8 +1556,14 @@ RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
indexform = (Form_pg_index) GETSTRUCT(locTuple);
indisvalid = indexform->indisvalid;
+ indisreplident = indexform->indisreplident;
ReleaseSysCache(locTuple);
+ if (indisreplident)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("could not drop index \"%s\" because it is required by the replica identity", rel->relname)));
+
/* Mark object as being an invalid index of system catalogs */
if (!indisvalid)
invalid_system_index = true;
diff --git a/src/test/regress/expected/replica_identity.out b/src/test/regress/expected/replica_identity.out
index e25ec06a84..56900451f7 100644
--- a/src/test/regress/expected/replica_identity.out
+++ b/src/test/regress/expected/replica_identity.out
@@ -227,6 +227,9 @@ Indexes:
-- used as replica identity.
ALTER TABLE test_replica_identity3 ALTER COLUMN id DROP NOT NULL;
ERROR: column "id" is in index used as replica identity
+-- DROP INDEX is not allowed if the index is used as REPLICA IDENTITY
+DROP INDEX test_replica_identity3_id_key;
+ERROR: could not drop index "test_replica_identity3_id_key" because it is required by the replica identity
DROP TABLE test_replica_identity;
DROP TABLE test_replica_identity2;
DROP TABLE test_replica_identity3;
diff --git a/src/test/regress/sql/replica_identity.sql b/src/test/regress/sql/replica_identity.sql
index 33da829713..f46769e965 100644
--- a/src/test/regress/sql/replica_identity.sql
+++ b/src/test/regress/sql/replica_identity.sql
@@ -98,6 +98,9 @@ ALTER TABLE test_replica_identity3 ALTER COLUMN id TYPE bigint;
-- used as replica identity.
ALTER TABLE test_replica_identity3 ALTER COLUMN id DROP NOT NULL;
+-- DROP INDEX is not allowed if the index is used as REPLICA IDENTITY
+DROP INDEX test_replica_identity3_id_key;
+
DROP TABLE test_replica_identity;
DROP TABLE test_replica_identity2;
DROP TABLE test_replica_identity3;
--
2.20.1
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-15 22:39 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-16 02:27 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 03:46 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 11:11 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-20 14:57 ` Re: Confused comment about drop replica identity index Euler Taveira <[email protected]>
@ 2021-12-21 00:46 ` Michael Paquier <[email protected]>
2021-12-30 06:45 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
1 sibling, 1 reply; 17+ messages in thread
From: Michael Paquier @ 2021-12-21 00:46 UTC (permalink / raw)
To: Euler Taveira <[email protected]>; +Cc: [email protected] <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Mon, Dec 20, 2021 at 11:57:32AM -0300, Euler Taveira wrote:
> What do you think about the attached patch? It forbids the DROP INDEX. We might
> add a detail message but I didn't in this patch.
Yeah. I'd agree about doing something like that on HEAD, and that
would help with some of the logirep-related patch currently being
worked on, as far as I understood.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* RE: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-15 22:39 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-16 02:27 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 03:46 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 11:11 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-20 14:57 ` Re: Confused comment about drop replica identity index Euler Taveira <[email protected]>
2021-12-21 00:46 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
@ 2021-12-30 06:45 ` [email protected] <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: [email protected] @ 2021-12-30 06:45 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Euler Taveira <[email protected]>; +Cc: [email protected] <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Tues, Dec 21, 2021 8:47 AM Michael Paquier <[email protected]> wrote:
> On Mon, Dec 20, 2021 at 11:57:32AM -0300, Euler Taveira wrote:
> > What do you think about the attached patch? It forbids the DROP INDEX.
> > We might add a detail message but I didn't in this patch.
>
> Yeah. I'd agree about doing something like that on HEAD, and that would help
> with some of the logirep-related patch currently being worked on, as far as I
> understood.
Hi,
I think forbids DROP INDEX might not completely solve this problem. Because
user could still use other command to delete the index, for example: ALTER
TABLE DROP COLUMN. After dropping the column, the index on it will also be
dropped.
Besides, user can also ALTER REPLICA IDENTITY USING INDEX "primary key", and in
this case, when they ALTER TABLE DROP CONSTR "PRIMARY KEY", the replica
identity index will also be dropped.
Best regards,
Hou zj
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-15 22:39 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-16 02:27 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 03:46 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 11:11 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-20 14:57 ` Re: Confused comment about drop replica identity index Euler Taveira <[email protected]>
@ 2021-12-22 07:40 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 17+ messages in thread
From: Michael Paquier @ 2021-12-22 07:40 UTC (permalink / raw)
To: Euler Taveira <[email protected]>; +Cc: [email protected] <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Mon, Dec 20, 2021 at 11:57:32AM -0300, Euler Taveira wrote:
> On Mon, Dec 20, 2021, at 8:11 AM, Michael Paquier wrote:
>> That's mostly fine. I have made some adjustments as per the
>> attached.
>
> Your patch looks good to me.
Thanks. I have done this part for now.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../YcLWYFP%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* RE: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-15 22:39 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-16 02:27 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 03:46 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 11:11 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
@ 2021-12-21 01:31 ` [email protected] <[email protected]>
1 sibling, 0 replies; 17+ messages in thread
From: [email protected] @ 2021-12-21 01:31 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Tue, Dec 20, 2021 at 19:11PM, Michael Paquier wrote:
> That's mostly fine. I have made some adjustments as per the attached.
Thanks for reviewing.
> + The default for non-system tables. Records the old values of the columns
> + of the primary key, if any. The default for non-system tables.
> The same sentence is repeated twice.
>
> + Records no information about the old row.(This is the
> default for system tables.)
> For consistency with the rest, this could drop the parenthesis for the second
> sentence.
>
> + <term><literal>USING INDEX index_name</literal></term>
> This should use <replaceable> as markup for index_name.
The change looks good to me.
Regards,
Wang wei
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
@ 2021-12-16 18:08 ` Alvaro Herrera <[email protected]>
2021-12-16 23:55 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
1 sibling, 1 reply; 17+ messages in thread
From: Alvaro Herrera @ 2021-12-16 18:08 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
On 2021-Dec-15, Michael Paquier wrote:
> On Tue, Dec 14, 2021 at 07:10:49PM +0530, Ashutosh Bapat wrote:
> > This code in RelationGetIndexList() is not according to that comment.
> >
> > if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex))
> > relation->rd_replidindex = pkeyIndex;
> > else if (replident == REPLICA_IDENTITY_INDEX && OidIsValid(candidateIndex))
> > relation->rd_replidindex = candidateIndex;
> > else
> > relation->rd_replidindex = InvalidOid;
>
> Yeah, the comment is wrong. If the index of a REPLICA_IDENTITY_INDEX
> is dropped, I recall that the behavior is the same as
> REPLICA_IDENTITY_NOTHING.
Hmm, so if a table has REPLICA IDENTITY INDEX and there is a publication
with an explicit column list, then we need to forbid the DROP INDEX for
that index.
I wonder why don't we just forbid DROP INDEX of an index that's been
defined as replica identity. It seems quite silly an operation to
allow.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"Ed is the standard text editor."
http://groups.google.com/group/alt.religion.emacs/msg/8d94ddab6a9b0ad3
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-16 18:08 ` Re: Confused comment about drop replica identity index Alvaro Herrera <[email protected]>
@ 2021-12-16 23:55 ` Michael Paquier <[email protected]>
2021-12-17 00:31 ` Re: Confused comment about drop replica identity index Euler Taveira <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Michael Paquier @ 2021-12-16 23:55 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Andres Freund <[email protected]>
On Thu, Dec 16, 2021 at 03:08:46PM -0300, Alvaro Herrera wrote:
> Hmm, so if a table has REPLICA IDENTITY INDEX and there is a publication
> with an explicit column list, then we need to forbid the DROP INDEX for
> that index.
Hmm. I have not followed this thread very closely.
> I wonder why don't we just forbid DROP INDEX of an index that's been
> defined as replica identity. It seems quite silly an operation to
> allow.
The commit logs talk about b23b0f55 here for this code, to ease the
handling of relcache entries for rd_replidindex. 07cacba is the
origin of the logic (see RelationGetIndexList). Andres?
I don't think that this is really an argument against putting more
restrictions as anything that deals with an index drop, including the
internal ones related to constraints, would need to go through
index_drop(), and new features may want more restrictions in place as
you say.
Now, I don't see a strong argument in changing this behavior either
(aka I have not looked at what this implies for the new publication
types), and we still need to do something for the comment/docs in
existing branches, anyway. So I would still fix this gap as a first
step, then deal with the rest on HEAD as necessary.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Confused comment about drop replica identity index
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-16 18:08 ` Re: Confused comment about drop replica identity index Alvaro Herrera <[email protected]>
2021-12-16 23:55 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
@ 2021-12-17 00:31 ` Euler Taveira <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Euler Taveira @ 2021-12-17 00:31 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Alvaro Herrera <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Andres Freund <[email protected]>
On Thu, Dec 16, 2021, at 8:55 PM, Michael Paquier wrote:
> On Thu, Dec 16, 2021 at 03:08:46PM -0300, Alvaro Herrera wrote:
> > Hmm, so if a table has REPLICA IDENTITY INDEX and there is a publication
> > with an explicit column list, then we need to forbid the DROP INDEX for
> > that index.
>
> Hmm. I have not followed this thread very closely.
>
> > I wonder why don't we just forbid DROP INDEX of an index that's been
> > defined as replica identity. It seems quite silly an operation to
> > allow.
It would avoid pilot errors.
> The commit logs talk about b23b0f55 here for this code, to ease the
> handling of relcache entries for rd_replidindex. 07cacba is the
> origin of the logic (see RelationGetIndexList). Andres?
>
> I don't think that this is really an argument against putting more
> restrictions as anything that deals with an index drop, including the
> internal ones related to constraints, would need to go through
> index_drop(), and new features may want more restrictions in place as
> you say.
>
> Now, I don't see a strong argument in changing this behavior either
> (aka I have not looked at what this implies for the new publication
> types), and we still need to do something for the comment/docs in
> existing branches, anyway. So I would still fix this gap as a first
> step, then deal with the rest on HEAD as necessary.
>
I've never understand the weak dependency between the REPLICA IDENTITY and the
index used by it. I'm afraid we will receive complaints about this unexpected
behavior (my logical replication setup is broken because I dropped an index) as
far as new logical replication features are added. Row filtering imposes some
restrictions in UPDATEs and DELETEs (an error message is returned and the
replication stops) if a column used in the expression isn't part of the REPLICA
IDENTITY anymore.
It seems we already have some code in RangeVarCallbackForDropRelation() that
deals with a system index error condition. We could save a syscall and provide
a test for indisreplident there.
If this restriction is undesirable, we should at least document this choice and
probably emit a WARNING for DROP INDEX.
--
Euler Taveira
EDB https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2021-12-30 06:45 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-06-27 16:58 [PATCH v1 1/8] Add jsonpath 'pg' modifier for enabling extensions Nikita Glukhov <[email protected]>
2021-12-14 12:38 Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-14 13:40 ` Re: Confused comment about drop replica identity index Ashutosh Bapat <[email protected]>
2021-12-15 03:24 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-15 09:18 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-15 22:39 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-16 02:27 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 03:46 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-20 11:11 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-20 14:57 ` Re: Confused comment about drop replica identity index Euler Taveira <[email protected]>
2021-12-21 00:46 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-30 06:45 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-22 07:40 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-21 01:31 ` RE: Confused comment about drop replica identity index [email protected] <[email protected]>
2021-12-16 18:08 ` Re: Confused comment about drop replica identity index Alvaro Herrera <[email protected]>
2021-12-16 23:55 ` Re: Confused comment about drop replica identity index Michael Paquier <[email protected]>
2021-12-17 00:31 ` Re: Confused comment about drop replica identity index Euler Taveira <[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