Hi Ajin/Nisha -- Here are my review comments for patch v15-0001 (code).
(AFAIK v16-0001 is the same as v15-0001, so this review is up to date)
Please also see the "nits" attachment to this post, which has many
more review comments of a more cosmetic nature.
======
src/backend/replication/logical/conflict.c
1.
+static const char *const ConflictResolverNames[] = {
+ [CR_APPLY_REMOTE] = "apply_remote",
+ [CR_KEEP_LOCAL] = "keep_local",
+ [CR_APPLY_OR_SKIP] = "apply_or_skip",
+ [CR_APPLY_OR_ERROR] = "apply_or_error",
+ [CR_SKIP] = "skip",
+ [CR_ERROR] = "error"
+};
+
Add missing static assertions:
StaticAssertDecl(lengthof(ConflictTypeNames) == CONFLICT_NUM_TYPES,
"array length mismatch");
StaticAssertDecl(lengthof(ConflictResolverNames) == CONFLICT_NUM_TYPES,
"array length mismatch");
~~~
2.
+#define CONFLICT_TYPE_MAX_RESOLVERS 4
Now unused. Remove this.
~~~
3.
+static const int ConflictTypeResolverMap[][CONFLICT_NUM_RESOLVERS + 1] = {
+ [CT_INSERT_EXISTS] = {CR_ERROR, CR_APPLY_REMOTE, CR_KEEP_LOCAL, -1},
+ [CT_UPDATE_EXISTS] = {CR_ERROR, CR_APPLY_REMOTE, CR_KEEP_LOCAL, -1},
+ [CT_UPDATE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR, -1},
+ [CT_UPDATE_MISSING] = {CR_SKIP, CR_APPLY_OR_SKIP, CR_APPLY_OR_ERROR,
CR_ERROR, -1},
+ [CT_DELETE_MISSING] = {CR_SKIP, CR_ERROR, -1},
+ [CT_DELETE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR, -1}
+};
If you are planning to keep this implementation of the Map, then that
-1 end of List marker is critical to the logic for the scanner part to
know when to stop looking for valid resolvers. So, I think you should
add another comment about that.
~~~
4.
+/*
+ * Report a warning about incomplete conflict detection and resolution if
+ * track_commit_timestamp is disabled.
+ */
+static void
+conf_detection_check_prerequisites(void)
+{
+ if (!track_commit_timestamp)
+ ereport(WARNING,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("conflict detection and resolution could be incomplete due to
disabled track_commit_timestamp"),
+ errdetail("Conflicts update_origin_differs and delete_origin_differs
cannot be "
+ "detected, and the origin and commit timestamp for the local row "
+ "will not be logged."));
+}
+
The "could be incomplete" wording seems vague to me. Why now just say
the WARNING reason in the errmsg?
SUGGESTION:
Conflicts types 'update_origin_differs' and 'delete_origin_differs'
cannot be detected unless "track_commit_timestamp" is enabled.
~~~
ParseAndGetSubConflictResolvers:
5.
+{
+ List *SeenTypes = NIL;
+ List *res = NIL;
All the list/string processing to determine if we have "seen" a
conflict type before looks inefficient to me. I had already
demonstrated in a previous review how this can be ditched in favour of
a simple boolean array. Again, I have implemented this in the NITPICKS
attachment to show how it can be implemented.
~~~
6.
+/*
+ * Get the list of conflict types and their corresponding default resolvers.
+ */
+List *
+GetDefaultConflictResolvers()
I'm wondering if it is worthwhile caching this default list. It will
never change, so if you cache it then you don't need to recalculate it
on subsequent calls.
======
src/bin/pg_dump/pg_dump.c
7.
Where are the test cases for the CONFLICT RESOLVER dump code?
~~~
getSubscriptions:
8.
+ ntups,
+ ntuples;
These var names are too similar. I can't tell them apart. Please
rename the new one (e.g. 'conf_ntuples').
~
9.
+ /* Initialize pointers in the list to NULL */
+ subinfo[i].conflict_resolver = (SimplePtrList)
+ {
+ 0
+ };
+
I didn't find anything else like this in PG source. IMO, it is better
to initialize both members explicitly to make it clear what this code
is actually doing. Or maybe memset() it.
SUGGESTION:
subinfo[i].conflict_resolver = (SimplePtrList)
{
.head = NULL, .tail = NULL
};
~~~
dumpSubscription:
+ /* Add conflict resolvers, if any */
+ if (fout->remoteVersion >= 180000)
+ {
+ bool first_resolver = true;
10a.
AFAICT this code is misplaced/broken. In the CREATE SUBSCRIPTION
syntax, the CONFLICT RESOLVER should come *before* any WITH clause, so
I think this is doomed to give a syntax error. The (missing) test
cases would have found this.
10b.
And, I expect when you fix that clause ordering then there will be
other fixes needed to correctly handle the closing parenthesis ')'.
~~~
11.
+/*
+ * destroyConflictResolverList - frees up the SimplePtrList containing
+ * cells pointing to struct ConflictTypeResolver nodes.
+ */
+static void
+destroyConcflictResolverList(SimplePtrList *conflictlist)
+{
+ SimplePtrListCell *cell = NULL;
+
+ /* Free the list items */
+ for (cell = conflictlist->head; cell; cell = cell->next)
+ pfree((ConflictTypeResolver *) cell->ptr);
+
+ /* Destroy the pointer list */
+ simple_ptr_list_destroy(conflictlist);
+}
Hmm. Is this even needed? AFAICT this entire function seems to be
doing the same as just calling simple_ptr_list_destroy(conflictlist)
directly.
======
src/bin/pg_dump/pg_dump.h
12.
+
+typedef struct ConflictTypeResolver
+{
+ const char *conflict_type;
+ const char *resolver;
+} ConflictTypeResolver;
+
This should be renamed (e.g. _ConflictTypeResolver or similar) with
underscore for consistency with other dump typedefs
~~~
13.
char *subfailover;
+ SimplePtrList conflict_resolver;
} SubscriptionInfo;
Rename this field to 'conflict_resolvers' (plural). Also, a comment
might help to say what the list elements are.
======
MISSING src/bin/psql/tab-complete.c
14.
Where is the tab-completion implementation for all the new syntax of
the v15 patch?
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Here are my v15-0001 (code/test) review comments which are "nits".
Basically, these are more likely to be cosmetic rather than functional, but
that doesn't mean I think they are unimportant.
======
src/backend/commands/subscriptioncmds.c
CreateSubscription:
N1.
+ /* Update the Conflict Resolvers in pg_subscription_conflict */
+ SetSubConflictResolvers(subid, conflict_resolvers);
nit - no need for uppercase /Conflict Resolvers/conflict resolvers/
nit - missing period in comments
~
AlterSubscription:
N2.
+ conflict_resolvers =
ParseAndGetSubConflictResolvers(
+
pstate,
+
stmt->resolvers,
+
false);
N2a.
The comment is inconsistent with other call to this same function. Either they
should both say "validate" or they both should not.
~
N2b.
nit - rearrange this for less harsh indenting.
~~~
N3.
DropSubscription:
+ /* Remove any associated conflict resolvers */
+ RemoveSubConflictResolvers(subid);
+
nit - add period to comment.
======
src/backend/parser/gram.y
N4.
+ ;
+conflict_type:
+ Sconst
{ $$ = $1; }
nit - add whiespace line above label.
======
src/backend/replication/logical/conflict.c
N5.
+ConflictType
+ValidateConflictType(const char *conflict_type)
+{
nit - lets call all the names, with suffix '_name'. So here would be
/conflict_type/conflict_type_name/
~~~
ValidateConflictTypeAndResolver:
N6.
+ConflictType
+ValidateConflictTypeAndResolver(const char *conflict_type,
+ const char
*conflict_resolver)
nit - lets call all the names, with suffix '_name'. So here would be:
/conflict_type/conflict_type_name/
/conflict_resolver/conflict_resolver_name/
~
N7.
+ if (!valid)
+ {
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("%s is not a valid conflict resolver",
conflict_resolver));
+ }
nit - Unnecessary { }.
~~~
ParseAndGetSubConflictResolvers:
N7.
+ /* Loop through the user provided resolvers */
+ foreach_ptr(DefElem, defel, stmtresolvers)
+ {
+ char *resolver;
+ ConflictTypeResolver *conftyperesolver = NULL;
nit - let's call names with '_name' suffix. s/resolver/resolver_name/
nit - let's consistently put a period on end of every comment.
~~~
UpdateSubConflictResolvers:
N8.
+void
+UpdateSubConflictResolvers(List *conflict_resolvers, Oid subid)
+{
+ Datum values[Natts_pg_subscription_conflict];
+ bool nulls[Natts_pg_subscription_conflict];
+ bool replaces[Natts_pg_subscription_conflict];
+ HeapTuple oldtup;
+ HeapTuple newtup = NULL;
+ Relation pg_subscription_conflict;
+ char *cur_conflict_res;
+ Datum datum;
nit - Many of those variables can be declared at a lower scope. See the nitpick
attachment.
nit - Put periods on all the comments in this func.
~~~
ResetSubConflictResolver:
N9.
+void
+ResetSubConflictResolver(Oid subid, char *conflict_type)
+{
nit - change the parameter /conflict_type/conflict_type_name/
nit - add periods a end of all comments
nit - cleanup some var names
~~~
N10.
+/*
+ * Set Conflict Resolvers on the subscription
+ */
+void
+SetSubConflictResolvers(Oid subId, List *conflict_resolvers)
nit - add period to function comment
nit - add period to the body comments too
nit - put some variables at a lower scope
~~~
N11.
+/*
+ * Remove the subscription conflict resolvers for the subscription id
+ */
+void
+RemoveSubConflictResolvers(Oid subid)
nit - add period to function comment.
nit - add period to other code comments.
======
src/bin/pg_dump/pg_dump.c
N12.
+static bool is_default_resolver(const char *confType, const char *confRes);
+static void destroyConcflictResolverList(SimplePtrList *list);
nit - improve the param names. /confType/conf_type_name/ and
/confRes/conf_resolver_name/
~~~
getSubscriptions:
N13.
nit - add periods to comments.
~
N14.
+ PQExpBuffer confQuery;
PGresult *res;
+ PGresult *confRes;
nit - let's rename these to "conf_query" amd "conf_res" (to match the existing
vars)
~
N15.
- ntups;
+ j,
nit - This 'j' can be a for-loop variable.
~
N16.
+ appendPQExpBuffer(confQuery,
+ "SELECT conftype, confres
FROM pg_catalog.pg_subscription_conflict "
+ "WHERE confsubid = %u order
by conftype;", subinfo[i].dobj.catId.oid);
+ confRes = ExecuteSqlQuery(fout, confQuery->data,
PGRES_TUPLES_OK);
"order by" should be uppercase.
~~~
N17.
+static bool
+is_default_resolver(const char *confType, const char *confRes)
nit - call this param '_name'
nit - this function is easier to read if you just expand out each conflict
type. (see nits attachment for what I mean)
~~~
N18:
+static void
+destroyConcflictResolverList(SimplePtrList *conflictlist)
nit - typo in this function name "Concflict"
======
src/bin/pg_dump/pg_dump.h
N19.
+
+typedef struct ConflictTypeResolver
+{
+ const char *conflict_type;
+ const char *resolver;
+} ConflictTypeResolver;
+
nit - remove excess spaces
nit - change the field name to be 'conflict_type_name' and
'conflict_resolver_name'
======
src/bin/psql/describe.c
describeSubscriptions:
N20.
+
+ /* Add conflict resolvers information from
pg_subscription_conflict */
+ if (pset.sversion >= 180000)
nit - add period to the comments.
======
src/include/nodes/parsenodes.h
N21.
+ List *resolvers; /* List of conflict resolvers */
} CreateSubscriptionStmt;
nit - this comment should be same as the other one in this file, and give a
proper description what these list element really are.
======
src/include/replication/conflict.h
N22.
+extern ConflictType ValidateConflictType(const char *conflict_type);
+extern ConflictType ValidateConflictTypeAndResolver(const char *conflict_type,
+
const char *conflict_resolver);
+extern List *GetDefaultConflictResolvers(void);
+extern void ResetSubConflictResolver(Oid subid, char *conflict_type);
nit - Those function parameters that are names should be called XXX_name. e.g.
/conflict_type/conflict_type_name/
/conflict_resolver/conflict_resolver_name/
======
src/test/regress/sql/subscription.sql
N23.
+-- creating subscription with no explicit conflict resolvers should
+-- configure default conflict resolvers
+CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false);
nit - add word "ok" in the comment.
~
N24.
+-- ok - valid conflict types and resolvers
+ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists =
'apply_remote', update_missing = 'skip', delete_origin_differs = 'keep_local' );
nit - add "alter with" in the comment
~
N25.
+-- ok - valid conflict types and resolvers
+ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (update_exists =
'keep_local', delete_missing = 'error', update_origin_differs = 'error');
nit - add "alter with" in the comment
Also, maybe you should say why there are multiple tests doing the same thing
(this and the previous one)
~
N26.
+-- fail - reset with an invalid conflit type
+ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER for 'foo';
nit - comment "reset for..."
nit - typo /conflit/conflict/
nit - use uppercase FOR
~
N27.
+-- ok - valid conflict type
+ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER for 'insert_exists';
nit - comment "reset for..."
nit - use uppercase FOR
======
FYI - Most of those nit review comments above have already been changed in the
accompanying nitpicks attachment.
======
Kind Regards,
Peter Smith.
Fujitsu Australia.diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 54d8ad9..1261d30 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -729,7 +729,7 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
recordDependencyOnOwner(SubscriptionRelationId, subid, owner);
- /* Update the Conflict Resolvers in pg_subscription_conflict */
+ /* Update the conflict resolvers in pg_subscription_conflict. */
SetSubConflictResolvers(subid, conflict_resolvers);
ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
sizeof(originname));
@@ -1598,10 +1598,8 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
* Get the list of conflict types and resolvers
and validate
* them.
*/
- conflict_resolvers =
ParseAndGetSubConflictResolvers(
-
pstate,
-
stmt->resolvers,
-
false);
+ conflict_resolvers =
+ ParseAndGetSubConflictResolvers(pstate,
stmt->resolvers, false);
/*
* Update the conflict resolvers for the
corresponding
@@ -1882,7 +1880,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool
isTopLevel)
/* Remove any associated relation synchronization states. */
RemoveSubscriptionRel(subid, InvalidOid);
- /* Remove any associated conflict resolvers */
+ /* Remove any associated conflict resolvers. */
RemoveSubConflictResolvers(subid);
/* Remove the origin tracking if exists. */
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 28591c9..68e0fa9 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10898,6 +10898,7 @@ AlterSubscriptionStmt:
$$ = (Node *) n;
}
;
+
conflict_type:
Sconst
{ $$ = $1; }
;
diff --git a/src/backend/replication/logical/conflict.c
b/src/backend/replication/logical/conflict.c
index eb46e39..a20a86e 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -40,6 +40,9 @@ static const char *const ConflictTypeNames[] = {
[CT_DELETE_MISSING] = "delete_missing"
};
+StaticAssertDecl(lengthof(ConflictTypeNames) == CONFLICT_NUM_TYPES,
+ "array length mismatch");
+
static const char *const ConflictResolverNames[] = {
[CR_APPLY_REMOTE] = "apply_remote",
[CR_KEEP_LOCAL] = "keep_local",
@@ -49,7 +52,8 @@ static const char *const ConflictResolverNames[] = {
[CR_ERROR] = "error"
};
-#define CONFLICT_TYPE_MAX_RESOLVERS 4
+StaticAssertDecl(lengthof(ConflictResolverNames) == CONFLICT_NUM_TYPES,
+ "array length mismatch");
/*
* Valid conflict resolvers for each conflict type.
@@ -562,7 +566,7 @@ conf_detection_check_prerequisites(void)
* Validate the conflict type and return the corresponding ConflictType enum.
*/
ConflictType
-ValidateConflictType(const char *conflict_type)
+ValidateConflictType(const char *conflict_type_name)
{
ConflictType type;
bool valid = false;
@@ -570,7 +574,7 @@ ValidateConflictType(const char *conflict_type)
/* Check conflict type validity */
for (type = 0; type < CONFLICT_NUM_TYPES; type++)
{
- if (pg_strcasecmp(ConflictTypeNames[type], conflict_type) == 0)
+ if (pg_strcasecmp(ConflictTypeNames[type], conflict_type_name)
== 0)
{
valid = true;
break;
@@ -580,30 +584,30 @@ ValidateConflictType(const char *conflict_type)
if (!valid)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("%s is not a valid conflict type",
conflict_type));
+ errmsg("%s is not a valid conflict type",
conflict_type_name));
return type;
}
/*
* Validate the conflict type and resolver. It returns an enum ConflictType
- * corresponding to the conflict type string passed by the caller.
+ * corresponding to the conflict type name passed by the caller.
*/
ConflictType
-ValidateConflictTypeAndResolver(const char *conflict_type,
- const char
*conflict_resolver)
+ValidateConflictTypeAndResolver(const char *conflict_type_name,
+ const char
*conflict_resolver_name)
{
ConflictType type;
ConflictResolver resolver;
bool valid = false;
/* Validate conflict type */
- type = ValidateConflictType(conflict_type);
+ type = ValidateConflictType(conflict_type_name);
/* Validate the conflict resolver name */
for (resolver = 0; resolver < CONFLICT_NUM_RESOLVERS; resolver++)
{
- if (pg_strcasecmp(ConflictResolverNames[resolver],
conflict_resolver) == 0)
+ if (pg_strcasecmp(ConflictResolverNames[resolver],
conflict_resolver_name) == 0)
{
valid = true;
break;
@@ -614,7 +618,7 @@ ValidateConflictTypeAndResolver(const char *conflict_type,
{
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("%s is not a valid conflict resolver",
conflict_resolver));
+ errmsg("%s is not a valid conflict resolver",
conflict_resolver_name));
}
/* Reset */
@@ -640,12 +644,13 @@ ValidateConflictTypeAndResolver(const char *conflict_type,
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%s is not a valid conflict resolver for
conflict type %s",
- conflict_resolver,
- conflict_type));
+ conflict_resolver_name,
+ conflict_type_name));
return type;
}
+
/*
* Common 'CONFLICT RESOLVER' parsing function for CREATE and ALTER
* SUBSCRIPTION commands.
@@ -660,36 +665,33 @@ List *
ParseAndGetSubConflictResolvers(ParseState *pstate,
List
*stmtresolvers, bool add_defaults)
{
- List *SeenTypes = NIL;
+ bool seen_type[CONFLICT_NUM_TYPES] = {0};
List *res = NIL;
- /* Loop through the user provided resolvers */
+ /* Loop through the user provided resolvers. */
foreach_ptr(DefElem, defel, stmtresolvers)
{
- char *resolver;
+ char *resolver_name;
ConflictTypeResolver *conftyperesolver = NULL;
+ ConflictType conflict_type;
- /* Check if the conflict type has already been seen */
- if (list_member(SeenTypes, makeString(defel->defname)))
- errorConflictingDefElem(defel, pstate);
+ /* Validate the conflict type and resolver. */
+ resolver_name = defGetString(defel);
+ resolver_name = downcase_truncate_identifier(resolver_name,
strlen(resolver_name), false);
+ conflict_type = ValidateConflictTypeAndResolver(defel->defname,
resolver_name);
- /* Validate the conflict type and resolver */
- resolver = defGetString(defel);
- resolver = downcase_truncate_identifier(
-
resolver, strlen(resolver), false);
- ValidateConflictTypeAndResolver(defel->defname,
-
resolver);
-
- /* Add the conflict type to the list of seen types */
- SeenTypes = lappend(SeenTypes, makeString(defel->defname));
+ /* Error if the conflict type has already been seen, else flag
it is as seen. */
+ if (seen_type[conflict_type])
+ errorConflictingDefElem(defel, pstate);
+ seen_type[conflict_type] = true;
conftyperesolver = palloc(sizeof(ConflictTypeResolver));
conftyperesolver->conflict_type_name = defel->defname;
- conftyperesolver->conflict_resolver_name = resolver;
+ conftyperesolver->conflict_resolver_name = resolver_name;
res = lappend(res, conftyperesolver);
}
- /* Once validation is complete, warn users if prerequisites are not met
*/
+ /* Once validation is complete, warn users if prerequisites are not
met. */
if (stmtresolvers)
conf_detection_check_prerequisites();
@@ -701,11 +703,10 @@ ParseAndGetSubConflictResolvers(ParseState *pstate,
{
for (int i = 0; i < CONFLICT_NUM_TYPES; i++)
{
- ConflictTypeResolver *conftyperesolver = NULL;
-
- if (!list_member(SeenTypes, makeString((char *)
ConflictTypeNames[i])))
+ if (!seen_type[i])
{
ConflictResolver def_resolver =
ConflictTypeResolverMap[i][0];
+ ConflictTypeResolver *conftyperesolver;
conftyperesolver =
palloc(sizeof(ConflictTypeResolver));
conftyperesolver->conflict_type_name =
ConflictTypeNames[i];
@@ -715,8 +716,6 @@ ParseAndGetSubConflictResolvers(ParseState *pstate,
}
}
- list_free_deep(SeenTypes);
-
return res;
}
@@ -726,7 +725,11 @@ ParseAndGetSubConflictResolvers(ParseState *pstate,
List *
GetDefaultConflictResolvers()
{
- List *res = NIL;
+ static List *res = NIL;
+
+ /* The defaults are always same, so return the same list. */
+ if (res)
+ return res;
for (ConflictType type = 0; type < CONFLICT_NUM_TYPES; type++)
{
@@ -756,13 +759,9 @@ UpdateSubConflictResolvers(List *conflict_resolvers, Oid
subid)
Datum values[Natts_pg_subscription_conflict];
bool nulls[Natts_pg_subscription_conflict];
bool replaces[Natts_pg_subscription_conflict];
- HeapTuple oldtup;
- HeapTuple newtup = NULL;
Relation pg_subscription_conflict;
- char *cur_conflict_res;
- Datum datum;
- /* Prepare to update a tuple */
+ /* Prepare to update a tuple. */
memset(nulls, false, sizeof(nulls));
memset(replaces, false, sizeof(replaces));
memset(values, 0, sizeof(values));
@@ -771,7 +770,12 @@ UpdateSubConflictResolvers(List *conflict_resolvers, Oid
subid)
foreach_ptr(ConflictTypeResolver, conftyperesolver, conflict_resolvers)
{
- /* Set up subid and conflict_type to search in cache */
+ HeapTuple oldtup;
+ HeapTuple newtup = NULL;
+ char *cur_conflict_res;
+ Datum datum;
+
+ /* Set up subid and conflict_type to search in cache. */
values[Anum_pg_subscription_conflict_confsubid - 1] =
ObjectIdGetDatum(subid);
values[Anum_pg_subscription_conflict_conftype - 1] =
CStringGetTextDatum(conftyperesolver->conflict_type_name);
@@ -795,7 +799,7 @@ UpdateSubConflictResolvers(List *conflict_resolvers, Oid
subid)
if (pg_strcasecmp(cur_conflict_res,
conftyperesolver->conflict_resolver_name) != 0)
{
- /* Update the new resolver */
+ /* Update the new resolver. */
values[Anum_pg_subscription_conflict_confres - 1] =
CStringGetTextDatum(conftyperesolver->conflict_resolver_name);
replaces[Anum_pg_subscription_conflict_confres - 1] =
true;
@@ -818,29 +822,28 @@ UpdateSubConflictResolvers(List *conflict_resolvers, Oid
subid)
* Reset the conflict resolver for this conflict type to its default setting.
*/
void
-ResetSubConflictResolver(Oid subid, char *conflict_type)
+ResetSubConflictResolver(Oid subid, char *conflict_type_name)
{
- ConflictType idx;
- ConflictTypeResolver *conflictResolver = NULL;
- List *conflictresolver_list = NIL;
-
- /* Validate the conflict type and get the index */
- idx = ValidateConflictType(conflict_type);
- conflictResolver = palloc(sizeof(ConflictTypeResolver));
- conflictResolver->conflict_type_name = conflict_type;
-
- /* Get the default resolver for this conflict_type */
- conflictResolver->conflict_resolver_name =
- ConflictResolverNames[ConflictTypeResolverMap[idx][0]];
-
- /* Create a list of conflict resolvers and update in catalog */
- conflictresolver_list = lappend(conflictresolver_list,
conflictResolver);
- UpdateSubConflictResolvers(conflictresolver_list, subid);
-
+ ConflictType conflict_type;
+ ConflictTypeResolver *ctr = NULL;
+ List *ctr_list = NIL;
+
+ /* Validate the conflict type name and get the conflict type. */
+ conflict_type = ValidateConflictType(conflict_type_name);
+ ctr = palloc(sizeof(ConflictTypeResolver));
+ ctr->conflict_type_name = conflict_type_name;
+
+ /* Get the default resolver for this conflict type. */
+ ctr->conflict_resolver_name =
+
ConflictResolverNames[ConflictTypeResolverMap[conflict_type][0]];
+
+ /* Create a (one-element) list of ConflictTypeResolver's and update the
catalog. */
+ ctr_list = lappend(ctr_list, ctr);
+ UpdateSubConflictResolvers(ctr_list, subid);
}
/*
- * Set Conflict Resolvers on the subscription
+ * Set Conflict Resolvers on the subscription.
*/
void
SetSubConflictResolvers(Oid subId, List *conflict_resolvers)
@@ -848,17 +851,18 @@ SetSubConflictResolvers(Oid subId, List
*conflict_resolvers)
Relation pg_subscription_conflict;
Datum values[Natts_pg_subscription_conflict];
bool nulls[Natts_pg_subscription_conflict];
- HeapTuple newtup = NULL;
- Oid conflict_oid;
pg_subscription_conflict = table_open(SubscriptionConflictId,
RowExclusiveLock);
- /* Prepare to update a tuple */
+ /* Prepare to update a tuple. */
memset(nulls, false, sizeof(nulls));
- /* Iterate over the list of resolvers */
+ /* Iterate over the list of resolvers. */
foreach_ptr(ConflictTypeResolver, conftyperesolver, conflict_resolvers)
{
+ HeapTuple newtup = NULL;
+ Oid conflict_oid;
+
values[Anum_pg_subscription_conflict_confsubid - 1] =
ObjectIdGetDatum(subId);
values[Anum_pg_subscription_conflict_conftype - 1] =
@@ -866,7 +870,7 @@ SetSubConflictResolvers(Oid subId, List *conflict_resolvers)
values[Anum_pg_subscription_conflict_confres - 1] =
CStringGetTextDatum(conftyperesolver->conflict_resolver_name);
- /* Get a new oid and update the tuple into catalog */
+ /* Get a new oid and update the tuple into catalog. */
conflict_oid = GetNewOidWithIndex(pg_subscription_conflict,
SubscriptionConflictOidIndexId,
Anum_pg_subscription_conflict_oid);
@@ -882,7 +886,7 @@ SetSubConflictResolvers(Oid subId, List *conflict_resolvers)
}
/*
- * Remove the subscription conflict resolvers for the subscription id
+ * Remove the subscription conflict resolvers for the subscription id.
*/
void
RemoveSubConflictResolvers(Oid subid)
@@ -906,7 +910,7 @@ RemoveSubConflictResolvers(Oid subid)
scan = table_beginscan_catalog(rel, 1, skey);
- /* Iterate through the tuples and delete them */
+ /* Iterate through the tuples and delete them. */
while (HeapTupleIsValid(tup = heap_getnext(scan, ForwardScanDirection)))
CatalogTupleDelete(rel, &tup->t_self);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 572eb52..860422e 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -395,7 +395,7 @@ static void setupDumpWorker(Archive *AH);
static TableInfo *getRootTableInfo(const TableInfo *tbinfo);
static bool forcePartitionRootLoad(const TableInfo *tbinfo);
static void read_dump_filters(const char *filename, DumpOptions *dopt);
-static bool is_default_resolver(const char *confType, const char *confRes);
+static bool is_default_resolver(const char *conf_type_name, const char
*conf_resolver_name);
static void destroyConcflictResolverList(SimplePtrList *list);
@@ -4832,9 +4832,9 @@ getSubscriptions(Archive *fout)
{
DumpOptions *dopt = fout->dopt;
PQExpBuffer query;
- PQExpBuffer confQuery;
+ PQExpBuffer conf_query;
PGresult *res;
- PGresult *confRes;
+ PGresult *conf_res;
SubscriptionInfo *subinfo;
int i_tableoid;
int i_oid;
@@ -4855,9 +4855,8 @@ getSubscriptions(Archive *fout)
int i_subenabled;
int i_subfailover;
int i,
- j,
ntups,
- ntuples;
+ conf_ntuples;
if (dopt->no_subscriptions || fout->remoteVersion < 100000)
return;
@@ -5018,36 +5017,36 @@ getSubscriptions(Archive *fout)
subinfo[i].subfailover =
pg_strdup(PQgetvalue(res, i, i_subfailover));
- /* Populate conflict type fields using the new query */
- confQuery = createPQExpBuffer();
- appendPQExpBuffer(confQuery,
+ /* Populate conflict type fields using the new query. */
+ conf_query = createPQExpBuffer();
+ appendPQExpBuffer(conf_query,
"SELECT conftype, confres
FROM pg_catalog.pg_subscription_conflict "
- "WHERE confsubid = %u order
by conftype;", subinfo[i].dobj.catId.oid);
- confRes = ExecuteSqlQuery(fout, confQuery->data,
PGRES_TUPLES_OK);
+ "WHERE confsubid = %u ORDER
BY conftype;", subinfo[i].dobj.catId.oid);
+ conf_res = ExecuteSqlQuery(fout, conf_query->data,
PGRES_TUPLES_OK);
- ntuples = PQntuples(confRes);
+ conf_ntuples = PQntuples(conf_res);
- /* Initialize pointers in the list to NULL */
- subinfo[i].conflict_resolver = (SimplePtrList)
+ /* Initialize pointers in the list to NULL. */
+ subinfo[i].conflict_resolvers = (SimplePtrList)
{
- 0
+ .head = NULL, .tail = NULL
};
- /* Store conflict types and resolvers from the query result in
subinfo */
- for (j = 0; j < ntuples; j++)
+ /* Store conflict types and resolvers from the query result in
subinfo. */
+ for (int j = 0; j < conf_ntuples; j++)
{
- /* Create the ConflictTypeResolver node */
- ConflictTypeResolver *conftyperesolver =
palloc(sizeof(ConflictTypeResolver));
+ /* Create the _ConflictTypeResolver node. */
+ _ConflictTypeResolver *ctr =
palloc(sizeof(_ConflictTypeResolver));
- conftyperesolver->conflict_type =
pg_strdup(PQgetvalue(confRes, j, 0));
- conftyperesolver->resolver =
pg_strdup(PQgetvalue(confRes, j, 1));
+ ctr->conflict_type_name =
pg_strdup(PQgetvalue(conf_res, j, 0));
+ ctr->conflict_resolver_name =
pg_strdup(PQgetvalue(conf_res, j, 1));
- /* Append the node to subinfo's list */
- simple_ptr_list_append(&subinfo[i].conflict_resolver,
conftyperesolver);
+ /* Append the node to subinfo's list. */
+ simple_ptr_list_append(&subinfo[i].conflict_resolvers,
ctr);
}
- PQclear(confRes);
- destroyPQExpBuffer(confQuery);
+ PQclear(conf_res);
+ destroyPQExpBuffer(conf_query);
/* Decide whether we want to dump it */
selectDumpableObject(&(subinfo[i].dobj), fout);
@@ -5299,26 +5298,24 @@ dumpSubscription(Archive *fout, const SubscriptionInfo
*subinfo)
{
bool first_resolver = true;
SimplePtrListCell *cell = NULL;
- ConflictTypeResolver *conftyperesolver = NULL;
- for (cell = subinfo->conflict_resolver.head; cell; cell =
cell->next)
+ for (cell = subinfo->conflict_resolvers.head; cell; cell =
cell->next)
{
- conftyperesolver = (ConflictTypeResolver *) cell->ptr;
+ _ConflictTypeResolver *ctr = (_ConflictTypeResolver *)
cell->ptr;
- if
(!is_default_resolver(conftyperesolver->conflict_type,
-
conftyperesolver->resolver))
+ if (!is_default_resolver(ctr->conflict_type_name,
ctr->conflict_resolver_name))
{
if (first_resolver)
{
appendPQExpBuffer(query, ") CONFLICT
RESOLVER (%s = '%s'",
-
conftyperesolver->conflict_type,
-
conftyperesolver->resolver);
+
ctr->conflict_type_name,
+
ctr->conflict_resolver_name);
first_resolver = false;
}
else
appendPQExpBuffer(query, ", %s = '%s'",
-
conftyperesolver->conflict_type,
-
conftyperesolver->resolver);
+
ctr->conflict_type_name,
+
ctr->conflict_resolver_name);
}
}
}
@@ -5382,7 +5379,7 @@ dumpSubscription(Archive *fout, const SubscriptionInfo
*subinfo)
subinfo->dobj.catId, 0,
subinfo->dobj.dumpId);
/* Clean-up the conflict_resolver list */
- destroyConcflictResolverList((SimplePtrList *)
&subinfo->conflict_resolver);
+ destroyConcflictResolverList((SimplePtrList *)
&subinfo->conflict_resolvers);
destroyPQExpBuffer(publications);
free(pubnames);
@@ -19126,7 +19123,7 @@ read_dump_filters(const char *filename, DumpOptions
*dopt)
* specified conflict type.
*/
static bool
-is_default_resolver(const char *confType, const char *confRes)
+is_default_resolver(const char *conf_type_name, const char *conf_resolver_name)
{
/*
* The default resolvers for each conflict type are taken from the
@@ -19136,15 +19133,23 @@ is_default_resolver(const char *confType, const char
*confRes)
* are changed.
*/
- if (strcmp(confType, "insert_exists") == 0 ||
- strcmp(confType, "update_exists") == 0)
- return strcmp(confRes, "error") == 0;
- else if (strcmp(confType, "update_missing") == 0 ||
- strcmp(confType, "delete_missing") == 0)
- return strcmp(confRes, "skip") == 0;
- else if (strcmp(confType, "update_origin_differs") == 0 ||
- strcmp(confType, "delete_origin_differs") == 0)
- return strcmp(confRes, "apply_remote") == 0;
+ if (strcmp(conf_type_name, "insert_exists") == 0)
+ return strcmp(conf_resolver_name, "error") == 0;
+
+ else if (strcmp(conf_type_name, "update_exists") == 0)
+ return strcmp(conf_resolver_name, "error") == 0;
+
+ else if (strcmp(conf_type_name, "update_missing") == 0)
+ return strcmp(conf_resolver_name, "skip") == 0;
+
+ else if (strcmp(conf_type_name, "update_origin_differs") == 0)
+ return strcmp(conf_resolver_name, "apply_remote") == 0;
+
+ else if (strcmp(conf_type_name, "delete_missing") == 0)
+ return strcmp(conf_resolver_name, "skip") == 0;
+
+ else if (strcmp(conf_type_name, "delete_origin_differs") == 0)
+ return strcmp(conf_resolver_name, "apply_remote") == 0;
return false;
}
@@ -19160,7 +19165,7 @@ destroyConcflictResolverList(SimplePtrList
*conflictlist)
/* Free the list items */
for (cell = conflictlist->head; cell; cell = cell->next)
- pfree((ConflictTypeResolver *) cell->ptr);
+ pfree((_ConflictTypeResolver *) cell->ptr);
/* Destroy the pointer list */
simple_ptr_list_destroy(conflictlist);
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index bb16e8b..5c69ad9 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -655,12 +655,11 @@ typedef struct _PublicationSchemaInfo
/*
* The SubscriptionInfo struct is used to represent subscription.
*/
-
-typedef struct ConflictTypeResolver
+typedef struct _ConflictTypeResolver
{
- const char *conflict_type;
- const char *resolver;
-} ConflictTypeResolver;
+ const char *conflict_type_name;
+ const char *conflict_resolver_name;
+} _ConflictTypeResolver;
typedef struct _SubscriptionInfo
{
@@ -680,7 +679,7 @@ typedef struct _SubscriptionInfo
char *suborigin;
char *suboriginremotelsn;
char *subfailover;
- SimplePtrList conflict_resolver;
+ SimplePtrList conflict_resolvers;
} SubscriptionInfo;
/*
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 8599f22..ee37043 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6628,7 +6628,7 @@ describeSubscriptions(const char *pattern, bool verbose)
", subskiplsn AS
\"%s\"\n",
gettext_noop("Skip
LSN"));
- /* Add conflict resolvers information from
pg_subscription_conflict */
+ /* Add conflict resolvers information from
pg_subscription_conflict. */
if (pset.sversion >= 180000)
appendPQExpBuffer(&buf,
", (SELECT
string_agg(conftype || ' = ' || confres, ', ' \n"
diff --git a/src/test/regress/expected/subscription.out
b/src/test/regress/expected/subscription.out
index 9791141..9c3a950 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -411,12 +411,12 @@ ERROR: foo is not a valid conflict resolver
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub
CONFLICT RESOLVER (update_missing = 'apply_remote') WITH (connect = false);
ERROR: apply_remote is not a valid conflict resolver for conflict type
update_missing
--- fail - duplicate conflict type
+-- fail - duplicate conflict types
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub CONFLICT RESOLVER (insert_exists = 'keep_local',
insert_exists = 'keep_local');
ERROR: conflicting or redundant options
LINE 1: ... CONFLICT RESOLVER (insert_exists = 'keep_local', insert_exi...
^
--- creating subscription with no explicit conflict resolvers should
+-- ok - creating subscription with no explicit conflict resolvers should
-- configure default conflict resolvers
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false);
WARNING: subscription was created, but is not connected
@@ -444,18 +444,18 @@ HINT: To initiate replication, you must manually create
the replication slot, e
regress_testsub | regress_subscription_user | f | {testpub} | f
| off | d | f | any | t
| f | f | off | dbname=regress_doesnotexist |
0/0 | delete_missing = skip, delete_origin_differs = keep_local,
insert_exists = keep_local, update_exists = error, update_missing = skip,
update_origin_differs = apply_remote
(1 row)
--- fail - altering with invalid conflict type
+-- fail - alter with invalid conflict type
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (foo = 'keep_local');
ERROR: foo is not a valid conflict type
--- fail - altering with invalid conflict resolver
+-- fail - alter with invalid conflict resolver
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists = 'foo');
ERROR: foo is not a valid conflict resolver
--- fail - altering with duplicate conflict type
+-- fail - alter with duplicate conflict types
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists =
'apply_remote', insert_exists = 'apply_remote');
ERROR: conflicting or redundant options
LINE 1: ...ONFLICT RESOLVER (insert_exists = 'apply_remote', insert_exi...
^
--- ok - valid conflict types and resolvers
+-- ok - alter with valid conflict types and resolvers
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists =
'apply_remote', update_missing = 'skip', delete_origin_differs = 'keep_local' );
WARNING: conflict detection and resolution could be incomplete due to
disabled track_commit_timestamp
DETAIL: Conflicts update_origin_differs and delete_origin_differs cannot be
detected, and the origin and commit timestamp for the local row will not be
logged.
@@ -466,7 +466,7 @@ DETAIL: Conflicts update_origin_differs and
delete_origin_differs cannot be det
regress_testsub | regress_subscription_user | f | {testpub} | f
| off | d | f | any | t
| f | f | off | dbname=regress_doesnotexist |
0/0 | delete_missing = skip, delete_origin_differs = keep_local,
insert_exists = apply_remote, update_exists = error, update_missing = skip,
update_origin_differs = apply_remote
(1 row)
--- ok - valid conflict types and resolvers
+-- ok - alter with valid conflict types and resolvers
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (update_exists =
'keep_local', delete_missing = 'error', update_origin_differs = 'error');
WARNING: conflict detection and resolution could be incomplete due to
disabled track_commit_timestamp
DETAIL: Conflicts update_origin_differs and delete_origin_differs cannot be
detected, and the origin and commit timestamp for the local row will not be
logged.
@@ -477,11 +477,11 @@ DETAIL: Conflicts update_origin_differs and
delete_origin_differs cannot be det
regress_testsub | regress_subscription_user | f | {testpub} | f
| off | d | f | any | t
| f | f | off | dbname=regress_doesnotexist |
0/0 | delete_missing = error, delete_origin_differs = keep_local,
insert_exists = apply_remote, update_exists = keep_local, update_missing =
skip, update_origin_differs = error
(1 row)
--- fail - reset with an invalid conflit type
-ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER for 'foo';
+-- fail - reset for invalid conflict type
+ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER FOR 'foo';
ERROR: foo is not a valid conflict type
--- ok - valid conflict type
-ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER for 'insert_exists';
+-- ok - reset for valid conflict type
+ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER FOR 'insert_exists';
\dRs+
List of subscriptions
Name | Owner | Enabled | Publication | Binary
| Streaming | Two-phase commit | Disable on error | Origin | Password required
| Run as owner? | Failover | Synchronous commit | Conninfo |
Skip LSN |
Conflict Resolvers
diff --git a/src/test/regress/sql/subscription.sql
b/src/test/regress/sql/subscription.sql
index 3f08869..9d42900 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -282,10 +282,10 @@ CREATE SUBSCRIPTION regress_testsub CONNECTION
'dbname=regress_doesnotexist' PUB
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub
CONFLICT RESOLVER (update_missing = 'apply_remote') WITH (connect = false);
--- fail - duplicate conflict type
+-- fail - duplicate conflict types
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub CONFLICT RESOLVER (insert_exists = 'keep_local',
insert_exists = 'keep_local');
--- creating subscription with no explicit conflict resolvers should
+-- ok - creating subscription with no explicit conflict resolvers should
-- configure default conflict resolvers
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false);
@@ -300,30 +300,30 @@ CREATE SUBSCRIPTION regress_testsub CONNECTION
'dbname=regress_doesnotexist' PUB
--check if above are configured; for non specified conflict types, default
resolvers should be seen
\dRs+
--- fail - altering with invalid conflict type
+-- fail - alter with invalid conflict type
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (foo = 'keep_local');
--- fail - altering with invalid conflict resolver
+-- fail - alter with invalid conflict resolver
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists = 'foo');
--- fail - altering with duplicate conflict type
+-- fail - alter with duplicate conflict types
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists =
'apply_remote', insert_exists = 'apply_remote');
--- ok - valid conflict types and resolvers
+-- ok - alter with valid conflict types and resolvers
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists =
'apply_remote', update_missing = 'skip', delete_origin_differs = 'keep_local' );
\dRs+
--- ok - valid conflict types and resolvers
+-- ok - alter with valid conflict types and resolvers
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (update_exists =
'keep_local', delete_missing = 'error', update_origin_differs = 'error');
\dRs+
--- fail - reset with an invalid conflit type
-ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER for 'foo';
+-- fail - reset for invalid conflict type
+ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER FOR 'foo';
--- ok - valid conflict type
-ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER for 'insert_exists';
+-- ok - reset for valid conflict type
+ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER FOR 'insert_exists';
\dRs+
