($INBOX_DIR/description missing)
help / color / mirror / Atom feed[PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
41+ messages / 7 participants
[nested] [flat]
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks
@ 2021-01-13 11:53 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Bharath Rupireddy @ 2021-01-13 11:53 UTC (permalink / raw)
Currently, in logical replication, relation map cache in the
subscriber is not getting invalidated when anything changes in
pg_subscription_rel or pg_subscription catalogues. So we end up
not reading the latest system catalogues always in logicalrep_rel_open
using GetSubscriptionRelState.
To fix this, invalidate the relation map cache entries in
invalidate_syncing_table_states and subscription_change_cb which
are invalidation callbacks for pg_subscription_rel and pg_subscription
catalogues respectively.
---
src/backend/replication/logical/relation.c | 26 +++++++++++++++++++++
src/backend/replication/logical/tablesync.c | 3 +++
src/backend/replication/logical/worker.c | 3 +++
src/include/replication/logicalrelation.h | 1 +
4 files changed, 33 insertions(+)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e861c0ff80..88ac772444 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -96,6 +96,32 @@ logicalrep_relmap_invalidate_cb(Datum arg, Oid reloid)
}
}
+/*
+ * Invalidate relation map cache whenever syscache of pg_subscription_rel or
+ * pg_subscription gets changed.
+ */
+void
+logicalrep_relmap_invalidate(void)
+{
+ LogicalRepRelMapEntry *entry;
+ HASH_SEQ_STATUS status;
+
+ /* Just to be sure. */
+ if (LogicalRepRelMap == NULL)
+ return;
+
+ /*
+ * There is no way to find the cache entry for which the syscache has been
+ * changed, so we mark all the cache entries state as unknown. Because of
+ * this, in logicalrep_rel_open the cache entry state will be read from
+ * the system catalogue using GetSubscriptionRelState.
+ */
+ hash_seq_init(&status, LogicalRepRelMap);
+
+ while ((entry = (LogicalRepRelMapEntry *) hash_seq_search(&status)) != NULL)
+ entry->state = SUBREL_STATE_UNKNOWN;
+}
+
/*
* Initialize the relation map cache.
*/
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196fd7..6b29173a5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -257,6 +257,9 @@ void
invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
{
table_states_valid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f2b2549a51..347264e4a8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2487,6 +2487,9 @@ static void
subscription_change_cb(Datum arg, int cacheid, uint32 hashvalue)
{
MySubscriptionValid = false;
+
+ /* Invalidate relation map cache. */
+ logicalrep_relmap_invalidate();
}
/*
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index 3f0b3deefb..f5e70b75b0 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -49,4 +49,5 @@ extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
extern char *logicalrep_typmap_gettypname(Oid remoteid);
+extern void logicalrep_relmap_invalidate(void);
#endif /* LOGICALRELATION_H */
--
2.30.0
--=-=-=--
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
@ 2024-04-23 14:57 Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Tom Lane @ 2024-04-23 14:57 UTC (permalink / raw)
To: Guo, Adam <[email protected]>; +Cc: [email protected] <[email protected]>
"Guo, Adam" <[email protected]> writes:
> I would like to report an issue with the pg_trgm extension on
> cross-architecture replication scenarios. When an x86_64 standby
> server is replicating from an aarch64 primary server or vice versa,
> the gist_trgm_ops opclass returns different results on the primary
> and standby.
I do not think that is a supported scenario. Hash functions and
suchlike are not guaranteed to produce the same results on different
CPU architectures. As a quick example, I get
regression=# select hashfloat8(34);
hashfloat8
------------
21570837
(1 row)
on x86_64 but
postgres=# select hashfloat8(34);
hashfloat8
------------
-602898821
(1 row)
on ppc32 thanks to the endianness difference.
> Given that this has problem has come up before and seems likely to
> come up again, I'm curious what other broad solutions there might be
> to resolve it?
Reject as not a bug. Discourage people from thinking that physical
replication will work across architectures.
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
@ 2024-04-30 16:43 ` Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Alexander Korotkov @ 2024-04-30 16:43 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Guo, Adam <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 23, 2024 at 5:57 PM Tom Lane <[email protected]> wrote:
> "Guo, Adam" <[email protected]> writes:
> > I would like to report an issue with the pg_trgm extension on
> > cross-architecture replication scenarios. When an x86_64 standby
> > server is replicating from an aarch64 primary server or vice versa,
> > the gist_trgm_ops opclass returns different results on the primary
> > and standby.
>
> I do not think that is a supported scenario. Hash functions and
> suchlike are not guaranteed to produce the same results on different
> CPU architectures. As a quick example, I get
>
> regression=# select hashfloat8(34);
> hashfloat8
> ------------
> 21570837
> (1 row)
>
> on x86_64 but
>
> postgres=# select hashfloat8(34);
> hashfloat8
> ------------
> -602898821
> (1 row)
>
> on ppc32 thanks to the endianness difference.
Given this, should we try to do better with binary compatibility
checks using ControlFileData? AFAICS they are supposed to check if
the database cluster is binary compatible with the running
architecture. But it obviously allows incompatibilities.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
@ 2024-04-30 16:54 ` Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Tom Lane @ 2024-04-30 16:54 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Guo, Adam <[email protected]>; [email protected] <[email protected]>
Alexander Korotkov <[email protected]> writes:
> Given this, should we try to do better with binary compatibility
> checks using ControlFileData? AFAICS they are supposed to check if
> the database cluster is binary compatible with the running
> architecture. But it obviously allows incompatibilities.
Perhaps. pg_control already covers endianness, which I think
is the root of the hashing differences I showed. Adding a field
for char signedness feels a little weird, since it's not directly
a property of the bits-on-disk, but maybe we should.
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
@ 2024-04-30 17:02 ` Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Alexander Korotkov @ 2024-04-30 17:02 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Guo, Adam <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 30, 2024 at 7:54 PM Tom Lane <[email protected]> wrote:
> Alexander Korotkov <[email protected]> writes:
> > Given this, should we try to do better with binary compatibility
> > checks using ControlFileData? AFAICS they are supposed to check if
> > the database cluster is binary compatible with the running
> > architecture. But it obviously allows incompatibilities.
>
> Perhaps. pg_control already covers endianness, which I think
> is the root of the hashing differences I showed. Adding a field
> for char signedness feels a little weird, since it's not directly
> a property of the bits-on-disk, but maybe we should.
I agree that storing char signedness might seem weird. But it appears
that we already store indexes that depend on char signedness. So,
it's effectively property of bits-on-disk even though it affects
indirectly. Then I see two options to make the picture consistent.
1) Assume that char signedness is somehow a property of bits-on-disk
even though it's weird. Then pg_trgm indexes are correct, but we need
to store char signedness in pg_control.
2) Assume that char signedness is not a property of bits-on-disk.
Then pg_trgm indexes are buggy and need to be fixed.
What do you think?
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
@ 2024-04-30 17:29 ` Tom Lane <[email protected]>
2024-05-01 02:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 41+ messages in thread
From: Tom Lane @ 2024-04-30 17:29 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Guo, Adam <[email protected]>; [email protected] <[email protected]>
Alexander Korotkov <[email protected]> writes:
> I agree that storing char signedness might seem weird. But it appears
> that we already store indexes that depend on char signedness. So,
> it's effectively property of bits-on-disk even though it affects
> indirectly. Then I see two options to make the picture consistent.
> 1) Assume that char signedness is somehow a property of bits-on-disk
> even though it's weird. Then pg_trgm indexes are correct, but we need
> to store char signedness in pg_control.
> 2) Assume that char signedness is not a property of bits-on-disk.
> Then pg_trgm indexes are buggy and need to be fixed.
> What do you think?
The problem with option (2) is the assumption that pg_trgm's behavior
is the only bug of this kind, either now or in the future. I think
that's just about an impossible standard to meet, because there's no
realistic way to test whether char signedness is affecting things.
(Sure, you can compare results across platforms, but maybe you
just didn't test the right case.)
Also, the bigger picture here is the seeming assumption that "if
we change pg_trgm then it will be safe to replicate from x86 to
arm". I don't believe that that's a good idea and I'm unwilling
to promise that it will work, regardless of what we do about
char signedness. That being the case, I don't want to invest a
lot of effort in the signedness issue. Option (1) is clearly
a small change with little if any risk of future breakage.
Option (2) ... not so much.
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
@ 2024-05-01 02:45 ` Masahiko Sawada <[email protected]>
1 sibling, 0 replies; 41+ messages in thread
From: Masahiko Sawada @ 2024-05-01 02:45 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>
On Wed, May 1, 2024 at 2:29 AM Tom Lane <[email protected]> wrote:
>
> Alexander Korotkov <[email protected]> writes:
> > I agree that storing char signedness might seem weird. But it appears
> > that we already store indexes that depend on char signedness. So,
> > it's effectively property of bits-on-disk even though it affects
> > indirectly. Then I see two options to make the picture consistent.
> > 1) Assume that char signedness is somehow a property of bits-on-disk
> > even though it's weird. Then pg_trgm indexes are correct, but we need
> > to store char signedness in pg_control.
> > 2) Assume that char signedness is not a property of bits-on-disk.
> > Then pg_trgm indexes are buggy and need to be fixed.
> > What do you think?
>
> The problem with option (2) is the assumption that pg_trgm's behavior
> is the only bug of this kind, either now or in the future. I think
> that's just about an impossible standard to meet, because there's no
> realistic way to test whether char signedness is affecting things.
> (Sure, you can compare results across platforms, but maybe you
> just didn't test the right case.)
>
> Also, the bigger picture here is the seeming assumption that "if
> we change pg_trgm then it will be safe to replicate from x86 to
> arm". I don't believe that that's a good idea and I'm unwilling
> to promise that it will work, regardless of what we do about
> char signedness. That being the case, I don't want to invest a
> lot of effort in the signedness issue.
I think that the char signedness issue is an issue also for developers
(and extension authors) since it could lead to confusion and potential
bugs in the future due to that. x86 developers would think of char as
always being signed and write code that will misbehave on arm
machines. For example, since logical replication should behave
correctly even in cross-arch replication all developers need to be
aware of that. I thought of using the -funsigned-char (or
-fsigned-char) compiler flag to avoid that but it would have a broader
impact not only on indexes.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
@ 2024-05-03 12:20 ` Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
1 sibling, 1 reply; 41+ messages in thread
From: Peter Eisentraut @ 2024-05-03 12:20 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Guo, Adam <[email protected]>; [email protected] <[email protected]>
On 30.04.24 19:29, Tom Lane wrote:
>> 1) Assume that char signedness is somehow a property of bits-on-disk
>> even though it's weird. Then pg_trgm indexes are correct, but we need
>> to store char signedness in pg_control.
>> 2) Assume that char signedness is not a property of bits-on-disk.
>> Then pg_trgm indexes are buggy and need to be fixed.
>> What do you think?
> Also, the bigger picture here is the seeming assumption that "if
> we change pg_trgm then it will be safe to replicate from x86 to
> arm". I don't believe that that's a good idea and I'm unwilling
> to promise that it will work, regardless of what we do about
> char signedness. That being the case, I don't want to invest a
> lot of effort in the signedness issue. Option (1) is clearly
> a small change with little if any risk of future breakage.
But note that option 1 would prevent some replication that is currently
working.
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
@ 2024-05-03 14:13 ` Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Tom Lane @ 2024-05-03 14:13 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>
Peter Eisentraut <[email protected]> writes:
> On 30.04.24 19:29, Tom Lane wrote:
>> Also, the bigger picture here is the seeming assumption that "if
>> we change pg_trgm then it will be safe to replicate from x86 to
>> arm". I don't believe that that's a good idea and I'm unwilling
>> to promise that it will work, regardless of what we do about
>> char signedness. That being the case, I don't want to invest a
>> lot of effort in the signedness issue. Option (1) is clearly
>> a small change with little if any risk of future breakage.
> But note that option 1 would prevent some replication that is currently
> working.
The point of this thread though is that it's working only for small
values of "work". People are rightfully unhappy if it seems to work
and then later they get bitten by compatibility problems.
Treating char signedness as a machine property in pg_control would
signal that we don't intend to make it work, and would ensure that
even the most minimal testing would find out that it doesn't work.
If we do not do that, it seems to me we have to buy into making
it work. That would mean dealing with the consequences of an
incompatible change in pg_trgm indexes, and then going through
the same dance again the next time(s) similar problems are found.
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
@ 2024-05-03 18:44 ` Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Peter Eisentraut @ 2024-05-03 18:44 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>
On 03.05.24 16:13, Tom Lane wrote:
> Peter Eisentraut <[email protected]> writes:
>> On 30.04.24 19:29, Tom Lane wrote:
>>> Also, the bigger picture here is the seeming assumption that "if
>>> we change pg_trgm then it will be safe to replicate from x86 to
>>> arm". I don't believe that that's a good idea and I'm unwilling
>>> to promise that it will work, regardless of what we do about
>>> char signedness. That being the case, I don't want to invest a
>>> lot of effort in the signedness issue. Option (1) is clearly
>>> a small change with little if any risk of future breakage.
>
>> But note that option 1 would prevent some replication that is currently
>> working.
>
> The point of this thread though is that it's working only for small
> values of "work". People are rightfully unhappy if it seems to work
> and then later they get bitten by compatibility problems.
>
> Treating char signedness as a machine property in pg_control would
> signal that we don't intend to make it work, and would ensure that
> even the most minimal testing would find out that it doesn't work.
>
> If we do not do that, it seems to me we have to buy into making
> it work. That would mean dealing with the consequences of an
> incompatible change in pg_trgm indexes, and then going through
> the same dance again the next time(s) similar problems are found.
Yes, that is understood. But anecdotally, replicating between x86-64
arm64 is occasionally used for upgrades or migrations. In practice,
this appears to have mostly worked. If we now discover that it won't
work with certain index extension modules, it's usable for most users.
Even if we say, you have to reindex everything afterwards, it's probably
still useful for these scenarios.
The way I understand the original report, the issue has to do
specifically with how signed and unsigned chars compare differently. I
don't imagine this is used anywhere in the table/heap code. So it's
plausible that this issue is really contained to indexes.
On the other hand, if we put in a check against this, then at least we
can answer any user questions about this with more certainty: No, won't
work, here is why.
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
@ 2024-05-03 22:36 ` Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Joe Conway @ 2024-05-03 22:36 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Masahiko Sawada <[email protected]>; Jim Mlodgenski <[email protected]>
On 5/3/24 11:44, Peter Eisentraut wrote:
> On 03.05.24 16:13, Tom Lane wrote:
>> Peter Eisentraut <[email protected]> writes:
>>> On 30.04.24 19:29, Tom Lane wrote:
>>>> Also, the bigger picture here is the seeming assumption that "if
>>>> we change pg_trgm then it will be safe to replicate from x86 to
>>>> arm". I don't believe that that's a good idea and I'm unwilling
>>>> to promise that it will work, regardless of what we do about
>>>> char signedness. That being the case, I don't want to invest a
>>>> lot of effort in the signedness issue. Option (1) is clearly
>>>> a small change with little if any risk of future breakage.
>>
>>> But note that option 1 would prevent some replication that is currently
>>> working.
>>
>> The point of this thread though is that it's working only for small
>> values of "work". People are rightfully unhappy if it seems to work
>> and then later they get bitten by compatibility problems.
>>
>> Treating char signedness as a machine property in pg_control would
>> signal that we don't intend to make it work, and would ensure that
>> even the most minimal testing would find out that it doesn't work.
>>
>> If we do not do that, it seems to me we have to buy into making
>> it work. That would mean dealing with the consequences of an
>> incompatible change in pg_trgm indexes, and then going through
>> the same dance again the next time(s) similar problems are found.
>
> Yes, that is understood. But anecdotally, replicating between x86-64 arm64 is
> occasionally used for upgrades or migrations. In practice, this appears to have
> mostly worked. If we now discover that it won't work with certain index
> extension modules, it's usable for most users. Even if we say, you have to
> reindex everything afterwards, it's probably still useful for these scenarios.
+1
I have heard similar anecdotes, and the reported experience goes even further --
many such upgrade/migration uses, with exceedingly rare reported failures.
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
@ 2024-05-15 05:56 ` Masahiko Sawada <[email protected]>
2024-05-18 21:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Masahiko Sawada @ 2024-05-15 05:56 UTC (permalink / raw)
To: Joe Conway <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Jim Mlodgenski <[email protected]>
On Sat, May 4, 2024 at 7:36 AM Joe Conway <[email protected]> wrote:
>
> On 5/3/24 11:44, Peter Eisentraut wrote:
> > On 03.05.24 16:13, Tom Lane wrote:
> >> Peter Eisentraut <[email protected]> writes:
> >>> On 30.04.24 19:29, Tom Lane wrote:
> >>>> Also, the bigger picture here is the seeming assumption that "if
> >>>> we change pg_trgm then it will be safe to replicate from x86 to
> >>>> arm". I don't believe that that's a good idea and I'm unwilling
> >>>> to promise that it will work, regardless of what we do about
> >>>> char signedness. That being the case, I don't want to invest a
> >>>> lot of effort in the signedness issue. Option (1) is clearly
> >>>> a small change with little if any risk of future breakage.
> >>
> >>> But note that option 1 would prevent some replication that is currently
> >>> working.
> >>
> >> The point of this thread though is that it's working only for small
> >> values of "work". People are rightfully unhappy if it seems to work
> >> and then later they get bitten by compatibility problems.
> >>
> >> Treating char signedness as a machine property in pg_control would
> >> signal that we don't intend to make it work, and would ensure that
> >> even the most minimal testing would find out that it doesn't work.
> >>
> >> If we do not do that, it seems to me we have to buy into making
> >> it work. That would mean dealing with the consequences of an
> >> incompatible change in pg_trgm indexes, and then going through
> >> the same dance again the next time(s) similar problems are found.
> >
> > Yes, that is understood. But anecdotally, replicating between x86-64 arm64 is
> > occasionally used for upgrades or migrations. In practice, this appears to have
> > mostly worked. If we now discover that it won't work with certain index
> > extension modules, it's usable for most users. Even if we say, you have to
> > reindex everything afterwards, it's probably still useful for these scenarios.
>
> +1
+1
How about extending amcheck to support GIN and GIst indexes so that it
can detect potential data incompatibility due to changing 'char' to
'unsigned char'? I think these new tests would be useful also for
users to check if they really need to reindex indexes due to such
changes. Also we fix pg_trgm so that it uses 'unsigned char' in PG18.
Users who upgraded to PG18 can run the new amcheck tests on the
primary as well as the standby.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
@ 2024-05-18 21:45 ` Noah Misch <[email protected]>
2024-08-29 20:48 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Noah Misch @ 2024-05-18 21:45 UTC (permalink / raw)
To: Masahiko Sawada <[email protected]>; +Cc: Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Jim Mlodgenski <[email protected]>
On Wed, May 15, 2024 at 02:56:54PM +0900, Masahiko Sawada wrote:
> On Sat, May 4, 2024 at 7:36 AM Joe Conway <[email protected]> wrote:
> > On 5/3/24 11:44, Peter Eisentraut wrote:
> > > On 03.05.24 16:13, Tom Lane wrote:
> > >> Peter Eisentraut <[email protected]> writes:
> > >>> On 30.04.24 19:29, Tom Lane wrote:
> > >>>> Also, the bigger picture here is the seeming assumption that "if
> > >>>> we change pg_trgm then it will be safe to replicate from x86 to
> > >>>> arm". I don't believe that that's a good idea and I'm unwilling
> > >>>> to promise that it will work, regardless of what we do about
> > >>>> char signedness. That being the case, I don't want to invest a
> > >>>> lot of effort in the signedness issue. Option (1) is clearly
> > >>>> a small change with little if any risk of future breakage.
> > >>
> > >>> But note that option 1 would prevent some replication that is currently
> > >>> working.
> > >>
> > >> The point of this thread though is that it's working only for small
> > >> values of "work". People are rightfully unhappy if it seems to work
> > >> and then later they get bitten by compatibility problems.
> > >>
> > >> Treating char signedness as a machine property in pg_control would
> > >> signal that we don't intend to make it work, and would ensure that
> > >> even the most minimal testing would find out that it doesn't work.
> > >>
> > >> If we do not do that, it seems to me we have to buy into making
> > >> it work. That would mean dealing with the consequences of an
> > >> incompatible change in pg_trgm indexes, and then going through
> > >> the same dance again the next time(s) similar problems are found.
> > >
> > > Yes, that is understood. But anecdotally, replicating between x86-64 arm64 is
> > > occasionally used for upgrades or migrations. In practice, this appears to have
> > > mostly worked. If we now discover that it won't work with certain index
> > > extension modules, it's usable for most users. Even if we say, you have to
> > > reindex everything afterwards, it's probably still useful for these scenarios.
Like you, I would not introduce a ControlFileData block for sign-of-char,
given the signs of breakage in extension indexing only. That would lose much
useful migration capability. I'm sympathetic to Tom's point, which I'll
attempt to summarize as: a ControlFileData block is a promise we know how to
keep, whereas we should expect further bug discoveries without it. At the
same time, I put more weight on the apparently-wide span of functionality that
works fine. (I wonder whether any static analyzer does or practically could
detect char sign dependence with a decent false positive rate.)
> +1
>
> How about extending amcheck to support GIN and GIst indexes so that it
> can detect potential data incompatibility due to changing 'char' to
> 'unsigned char'? I think these new tests would be useful also for
> users to check if they really need to reindex indexes due to such
> changes. Also we fix pg_trgm so that it uses 'unsigned char' in PG18.
> Users who upgraded to PG18 can run the new amcheck tests on the
> primary as well as the standby.
If I were standardizing pg_trgm on one or the other notion of "char", I would
choose signed char, since I think it's still the majority. More broadly, I
see these options to fix pg_trgm:
1. Change to signed char. Every arm64 system needs to scan pg_trgm indexes.
2. Change to unsigned char. Every x86 system needs to scan pg_trgm indexes.
3. Offer both, as an upgrade path. For example, pg_trgm could have separate
operator classes gin_trgm_ops and gin_trgm_ops_unsigned. Running
pg_upgrade on an unsigned-char system would automatically map v17
gin_trgm_ops to v18 gin_trgm_ops_unsigned. This avoids penalizing any
architecture with upgrade-time scans.
Independently, having amcheck for GIN and/or GiST would be great.
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-18 21:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
@ 2024-08-29 20:48 ` Masahiko Sawada <[email protected]>
2024-08-31 03:10 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Masahiko Sawada @ 2024-08-29 20:48 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Jim Mlodgenski <[email protected]>
On Sun, May 19, 2024 at 6:46 AM Noah Misch <[email protected]> wrote:
>
> On Wed, May 15, 2024 at 02:56:54PM +0900, Masahiko Sawada wrote:
> >
> > How about extending amcheck to support GIN and GIst indexes so that it
> > can detect potential data incompatibility due to changing 'char' to
> > 'unsigned char'? I think these new tests would be useful also for
> > users to check if they really need to reindex indexes due to such
> > changes. Also we fix pg_trgm so that it uses 'unsigned char' in PG18.
> > Users who upgraded to PG18 can run the new amcheck tests on the
> > primary as well as the standby.
>
> If I were standardizing pg_trgm on one or the other notion of "char", I would
> choose signed char, since I think it's still the majority. More broadly, I
> see these options to fix pg_trgm:
>
> 1. Change to signed char. Every arm64 system needs to scan pg_trgm indexes.
> 2. Change to unsigned char. Every x86 system needs to scan pg_trgm indexes.
Even though it's true that signed char systems are the majority, it
would not be acceptable to force the need to scan pg_trgm indexes on
unsigned char systems.
> 3. Offer both, as an upgrade path. For example, pg_trgm could have separate
> operator classes gin_trgm_ops and gin_trgm_ops_unsigned. Running
> pg_upgrade on an unsigned-char system would automatically map v17
> gin_trgm_ops to v18 gin_trgm_ops_unsigned. This avoids penalizing any
> architecture with upgrade-time scans.
Very interesting idea. How can new v18 users use the correct operator
class? I don't want to require users to specify the correct signed or
unsigned operator classes when creating a GIN index. Maybe we need to
dynamically use the correct compare function for the same operator
class depending on the char signedness. But is it possible to do it on
the extension (e.g. pg_trgm) side?
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-18 21:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-08-29 20:48 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
@ 2024-08-31 03:10 ` Noah Misch <[email protected]>
2024-09-06 21:07 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Noah Misch @ 2024-08-31 03:10 UTC (permalink / raw)
To: Masahiko Sawada <[email protected]>; +Cc: Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Jim Mlodgenski <[email protected]>
On Thu, Aug 29, 2024 at 03:48:53PM -0500, Masahiko Sawada wrote:
> On Sun, May 19, 2024 at 6:46 AM Noah Misch <[email protected]> wrote:
> > If I were standardizing pg_trgm on one or the other notion of "char", I would
> > choose signed char, since I think it's still the majority. More broadly, I
> > see these options to fix pg_trgm:
> >
> > 1. Change to signed char. Every arm64 system needs to scan pg_trgm indexes.
> > 2. Change to unsigned char. Every x86 system needs to scan pg_trgm indexes.
>
> Even though it's true that signed char systems are the majority, it
> would not be acceptable to force the need to scan pg_trgm indexes on
> unsigned char systems.
>
> > 3. Offer both, as an upgrade path. For example, pg_trgm could have separate
> > operator classes gin_trgm_ops and gin_trgm_ops_unsigned. Running
> > pg_upgrade on an unsigned-char system would automatically map v17
> > gin_trgm_ops to v18 gin_trgm_ops_unsigned. This avoids penalizing any
> > architecture with upgrade-time scans.
>
> Very interesting idea. How can new v18 users use the correct operator
> class? I don't want to require users to specify the correct signed or
> unsigned operator classes when creating a GIN index. Maybe we need to
In brief, it wouldn't matter which operator class new v18 indexes use. The
documentation would focus on gin_trgm_ops and also say something like:
There's an additional operator class, gin_trgm_ops_unsigned. It behaves
exactly like gin_trgm_ops, but it uses a deprecated on-disk representation.
Use gin_trgm_ops in new indexes, but there's no disadvantage from continuing
to use gin_trgm_ops_unsigned. Before PostgreSQL 18, gin_trgm_ops used a
platform-dependent representation. pg_upgrade automatically uses
gin_trgm_ops_unsigned when upgrading from source data that used the
deprecated representation.
What concerns might users have, then? (Neither operator class would use plain
"char" in a context that affects on-disk state. They'll use "signed char" and
"unsigned char".)
> dynamically use the correct compare function for the same operator
> class depending on the char signedness. But is it possible to do it on
> the extension (e.g. pg_trgm) side?
No, I don't think the extension can do that cleanly. It would need to store
the signedness in the index somehow, and GIN doesn't call the opclass at a
time facilitating that. That would need help from the core server.
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-18 21:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-08-29 20:48 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-08-31 03:10 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
@ 2024-09-06 21:07 ` Masahiko Sawada <[email protected]>
2024-09-06 21:59 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Masahiko Sawada @ 2024-09-06 21:07 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Jim Mlodgenski <[email protected]>
On Fri, Aug 30, 2024 at 8:10 PM Noah Misch <[email protected]> wrote:
>
> On Thu, Aug 29, 2024 at 03:48:53PM -0500, Masahiko Sawada wrote:
> > On Sun, May 19, 2024 at 6:46 AM Noah Misch <[email protected]> wrote:
> > > If I were standardizing pg_trgm on one or the other notion of "char", I would
> > > choose signed char, since I think it's still the majority. More broadly, I
> > > see these options to fix pg_trgm:
> > >
> > > 1. Change to signed char. Every arm64 system needs to scan pg_trgm indexes.
> > > 2. Change to unsigned char. Every x86 system needs to scan pg_trgm indexes.
> >
> > Even though it's true that signed char systems are the majority, it
> > would not be acceptable to force the need to scan pg_trgm indexes on
> > unsigned char systems.
> >
> > > 3. Offer both, as an upgrade path. For example, pg_trgm could have separate
> > > operator classes gin_trgm_ops and gin_trgm_ops_unsigned. Running
> > > pg_upgrade on an unsigned-char system would automatically map v17
> > > gin_trgm_ops to v18 gin_trgm_ops_unsigned. This avoids penalizing any
> > > architecture with upgrade-time scans.
> >
> > Very interesting idea. How can new v18 users use the correct operator
> > class? I don't want to require users to specify the correct signed or
> > unsigned operator classes when creating a GIN index. Maybe we need to
>
> In brief, it wouldn't matter which operator class new v18 indexes use. The
> documentation would focus on gin_trgm_ops and also say something like:
>
> There's an additional operator class, gin_trgm_ops_unsigned. It behaves
> exactly like gin_trgm_ops, but it uses a deprecated on-disk representation.
> Use gin_trgm_ops in new indexes, but there's no disadvantage from continuing
> to use gin_trgm_ops_unsigned. Before PostgreSQL 18, gin_trgm_ops used a
> platform-dependent representation. pg_upgrade automatically uses
> gin_trgm_ops_unsigned when upgrading from source data that used the
> deprecated representation.
>
> What concerns might users have, then? (Neither operator class would use plain
> "char" in a context that affects on-disk state. They'll use "signed char" and
> "unsigned char".)
I think I understand your idea now. Since gin_trgm_ops will use
"signed char", there is no impact for v18 users -- they can continue
using gin_trgm_ops.
But how does pg_upgrade use gin_trgm_ops_unsigned? This opclass will
be created by executing the pg_trgm script for v18, but it isn't
executed during pg_upgrade. Another way would be to do these opclass
replacement when executing the pg_trgm's update script (i.e., 1.6 to
1.7).
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-18 21:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-08-29 20:48 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-08-31 03:10 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-09-06 21:07 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
@ 2024-09-06 21:59 ` Noah Misch <[email protected]>
2024-09-06 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Noah Misch @ 2024-09-06 21:59 UTC (permalink / raw)
To: Masahiko Sawada <[email protected]>; +Cc: Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Jim Mlodgenski <[email protected]>
On Fri, Sep 06, 2024 at 02:07:10PM -0700, Masahiko Sawada wrote:
> On Fri, Aug 30, 2024 at 8:10 PM Noah Misch <[email protected]> wrote:
> > On Thu, Aug 29, 2024 at 03:48:53PM -0500, Masahiko Sawada wrote:
> > > On Sun, May 19, 2024 at 6:46 AM Noah Misch <[email protected]> wrote:
> > > > If I were standardizing pg_trgm on one or the other notion of "char", I would
> > > > choose signed char, since I think it's still the majority. More broadly, I
> > > > see these options to fix pg_trgm:
> > > >
> > > > 1. Change to signed char. Every arm64 system needs to scan pg_trgm indexes.
> > > > 2. Change to unsigned char. Every x86 system needs to scan pg_trgm indexes.
> > >
> > > Even though it's true that signed char systems are the majority, it
> > > would not be acceptable to force the need to scan pg_trgm indexes on
> > > unsigned char systems.
> > >
> > > > 3. Offer both, as an upgrade path. For example, pg_trgm could have separate
> > > > operator classes gin_trgm_ops and gin_trgm_ops_unsigned. Running
> > > > pg_upgrade on an unsigned-char system would automatically map v17
> > > > gin_trgm_ops to v18 gin_trgm_ops_unsigned. This avoids penalizing any
> > > > architecture with upgrade-time scans.
> > >
> > > Very interesting idea. How can new v18 users use the correct operator
> > > class? I don't want to require users to specify the correct signed or
> > > unsigned operator classes when creating a GIN index. Maybe we need to
> >
> > In brief, it wouldn't matter which operator class new v18 indexes use. The
> > documentation would focus on gin_trgm_ops and also say something like:
> >
> > There's an additional operator class, gin_trgm_ops_unsigned. It behaves
> > exactly like gin_trgm_ops, but it uses a deprecated on-disk representation.
> > Use gin_trgm_ops in new indexes, but there's no disadvantage from continuing
> > to use gin_trgm_ops_unsigned. Before PostgreSQL 18, gin_trgm_ops used a
> > platform-dependent representation. pg_upgrade automatically uses
> > gin_trgm_ops_unsigned when upgrading from source data that used the
> > deprecated representation.
> >
> > What concerns might users have, then? (Neither operator class would use plain
> > "char" in a context that affects on-disk state. They'll use "signed char" and
> > "unsigned char".)
>
> I think I understand your idea now. Since gin_trgm_ops will use
> "signed char", there is no impact for v18 users -- they can continue
> using gin_trgm_ops.
Right.
> But how does pg_upgrade use gin_trgm_ops_unsigned? This opclass will
> be created by executing the pg_trgm script for v18, but it isn't
> executed during pg_upgrade. Another way would be to do these opclass
> replacement when executing the pg_trgm's update script (i.e., 1.6 to
> 1.7).
Yes, that's one way to make it work. If we do it that way, it would be
critical to make the ALTER EXTENSION UPDATE run before anything uses the
index. Otherwise, we'll run new v18 "signed char" code on a v17 "unsigned
char" data file. Running ALTER EXTENSION UPDATE early enough should be
feasible, so that's fine. Some other options:
- If v18 "pg_dump -b" decides to emit CREATE INDEX ... gin_trgm_ops_unsigned,
then make it also emit the statements to create the opclass.
- Ship pg_trgm--1.6--1.7.sql in back branches. If the upgrade wants to use
gin_trgm_ops_unsigned, require the user to ALTER EXTENSION UPDATE first.
(In back branches, the C code behind gin_trgm_ops_unsigned could just raise
an error if called.)
What other options exist? I bet there are more.
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-18 21:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-08-29 20:48 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-08-31 03:10 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-09-06 21:07 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-09-06 21:59 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
@ 2024-09-06 22:36 ` Tom Lane <[email protected]>
2024-09-09 23:33 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Tom Lane @ 2024-09-06 22:36 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Jim Mlodgenski <[email protected]>
Noah Misch <[email protected]> writes:
> Yes, that's one way to make it work. If we do it that way, it would be
> critical to make the ALTER EXTENSION UPDATE run before anything uses the
> index. Otherwise, we'll run new v18 "signed char" code on a v17 "unsigned
> char" data file. Running ALTER EXTENSION UPDATE early enough should be
> feasible, so that's fine. Some other options:
> - If v18 "pg_dump -b" decides to emit CREATE INDEX ... gin_trgm_ops_unsigned,
> then make it also emit the statements to create the opclass.
> - Ship pg_trgm--1.6--1.7.sql in back branches. If the upgrade wants to use
> gin_trgm_ops_unsigned, require the user to ALTER EXTENSION UPDATE first.
> (In back branches, the C code behind gin_trgm_ops_unsigned could just raise
> an error if called.)
I feel like all of these are leaning heavily on users to get it right,
and therefore have a significant chance of breaking use-cases that
were perfectly fine before.
Remind me of why we can't do something like this:
* Add APIs that allow opclasses to read/write some space in the GIN
metapage. (We could get ambitious and add such APIs for other AMs
too, but doing it just for GIN is probably a prudent start.) Ensure
that this space is initially zeroed.
* In gin_trgm_ops, read a byte of this space and interpret it as
0 = unset
1 = signed chars
2 = unsigned chars
If the value is zero, set the byte on the basis of the native
char-signedness of the current platform. (Obviously, a
secondary server couldn't write the byte, and would have to
wait for the primary to update the value. In the meantime,
it's no more broken than today.)
* Subsequently, use either signed or unsigned comparisons
based on that value.
This would automatically do the right thing in all cases that
work today, and it'd provide the ability for cross-platform
replication to work in future. You can envision cases where
transferring a pre-existing index to a platform of the other
stripe would misbehave, but they're the same cases that fail
today, and the fix remains the same: reindex.
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-18 21:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-08-29 20:48 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-08-31 03:10 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-09-06 21:07 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-09-06 21:59 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-09-06 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
@ 2024-09-09 23:33 ` Masahiko Sawada <[email protected]>
2024-09-09 23:42 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Masahiko Sawada @ 2024-09-09 23:33 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Jim Mlodgenski <[email protected]>
On Fri, Sep 6, 2024 at 3:36 PM Tom Lane <[email protected]> wrote:
>
> Noah Misch <[email protected]> writes:
> > Yes, that's one way to make it work. If we do it that way, it would be
> > critical to make the ALTER EXTENSION UPDATE run before anything uses the
> > index. Otherwise, we'll run new v18 "signed char" code on a v17 "unsigned
> > char" data file. Running ALTER EXTENSION UPDATE early enough should be
> > feasible, so that's fine. Some other options:
>
> > - If v18 "pg_dump -b" decides to emit CREATE INDEX ... gin_trgm_ops_unsigned,
> > then make it also emit the statements to create the opclass.
>
> > - Ship pg_trgm--1.6--1.7.sql in back branches. If the upgrade wants to use
> > gin_trgm_ops_unsigned, require the user to ALTER EXTENSION UPDATE first.
> > (In back branches, the C code behind gin_trgm_ops_unsigned could just raise
> > an error if called.)
>
> I feel like all of these are leaning heavily on users to get it right,
> and therefore have a significant chance of breaking use-cases that
> were perfectly fine before.
>
> Remind me of why we can't do something like this:
>
> * Add APIs that allow opclasses to read/write some space in the GIN
> metapage. (We could get ambitious and add such APIs for other AMs
> too, but doing it just for GIN is probably a prudent start.) Ensure
> that this space is initially zeroed.
>
> * In gin_trgm_ops, read a byte of this space and interpret it as
> 0 = unset
> 1 = signed chars
> 2 = unsigned chars
> If the value is zero, set the byte on the basis of the native
> char-signedness of the current platform. (Obviously, a
> secondary server couldn't write the byte, and would have to
> wait for the primary to update the value. In the meantime,
> it's no more broken than today.)
When do we set the byte on the primary server? If it's the first time
to use the GIN index, secondary servers would have to wait for the
primary to use the GIN index, which could be an unpredictable time or
it may never come depending on index usages. Probably we can make
pg_upgrade set the byte in the meta page of GIN indexes that use the
gin_trgm_ops.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-18 21:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-08-29 20:48 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-08-31 03:10 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-09-06 21:07 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-09-06 21:59 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-09-06 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-09-09 23:33 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
@ 2024-09-09 23:42 ` Tom Lane <[email protected]>
2024-09-10 05:18 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Tom Lane @ 2024-09-09 23:42 UTC (permalink / raw)
To: Masahiko Sawada <[email protected]>; +Cc: Noah Misch <[email protected]>; Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Jim Mlodgenski <[email protected]>
Masahiko Sawada <[email protected]> writes:
> When do we set the byte on the primary server? If it's the first time
> to use the GIN index, secondary servers would have to wait for the
> primary to use the GIN index, which could be an unpredictable time or
> it may never come depending on index usages. Probably we can make
> pg_upgrade set the byte in the meta page of GIN indexes that use the
> gin_trgm_ops.
Hmm, perhaps. That plus set-it-during-index-create would remove the
need for dynamic update like I suggested. So very roughly the amount
of complexity would balance out. Do you have an idea for how we'd get
this to happen during pg_upgrade, exactly?
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-18 21:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-08-29 20:48 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-08-31 03:10 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-09-06 21:07 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-09-06 21:59 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-09-06 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-09-09 23:33 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-09-09 23:42 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
@ 2024-09-10 05:18 ` Masahiko Sawada <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Masahiko Sawada @ 2024-09-10 05:18 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; Alexander Korotkov <[email protected]>; Guo, Adam <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; Jim Mlodgenski <[email protected]>
On Mon, Sep 9, 2024 at 4:42 PM Tom Lane <[email protected]> wrote:
>
> Masahiko Sawada <[email protected]> writes:
> > When do we set the byte on the primary server? If it's the first time
> > to use the GIN index, secondary servers would have to wait for the
> > primary to use the GIN index, which could be an unpredictable time or
> > it may never come depending on index usages. Probably we can make
> > pg_upgrade set the byte in the meta page of GIN indexes that use the
> > gin_trgm_ops.
>
> Hmm, perhaps. That plus set-it-during-index-create would remove the
> need for dynamic update like I suggested. So very roughly the amount
> of complexity would balance out.
Yes, I think your set-it-during-index-create would be necessary.
> Do you have an idea for how we'd get
> this to happen during pg_upgrade, exactly?
What I was thinking is that we have "pg_dump --binary-upgrade" emit a
function call, say "SELECT binary_upgrade_update_gin_meta_page()" for
each GIN index, and the meta pages are updated when restoring the
schemas.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 41+ messages in thread
end of thread, other threads:[~2024-09-10 05:18 UTC | newest]
Thread overview: 41+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2021-01-13 11:53 [PATCH v3 2/2] Invalidate relation map cache in subscriber syscache invalidation callbacks Bharath Rupireddy <[email protected]>
2024-04-23 14:57 Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 16:43 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 16:54 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-04-30 17:02 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Alexander Korotkov <[email protected]>
2024-04-30 17:29 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-01 02:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-03 12:20 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 14:13 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-05-03 18:44 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Peter Eisentraut <[email protected]>
2024-05-03 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Joe Conway <[email protected]>
2024-05-15 05:56 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-05-18 21:45 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-08-29 20:48 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-08-31 03:10 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-09-06 21:07 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-09-06 21:59 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Noah Misch <[email protected]>
2024-09-06 22:36 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-09-09 23:33 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[email protected]>
2024-09-09 23:42 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Tom Lane <[email protected]>
2024-09-10 05:18 ` Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation Masahiko Sawada <[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