On Tue, Apr 5, 2022 at 4:08 PM Noah Misch <n...@leadboat.com> wrote:
>
> On Tue, Apr 05, 2022 at 03:05:10PM +0900, Masahiko Sawada wrote:
> > I've attached an updated patch. The patch includes a regression test
> > to detect the new violation as we discussed. I've confirmed that
> > Cirrus CI tests pass. Please confirm on AIX and review the patch.
>
> When the context of a "git grep skiplsn" match involves several struct fields
> in struct order, please change to the new order. In other words, do for all
> "git grep skiplsn" matches what the v2 patch does in GetSubscription(). The
> v2 patch does not do this for catalogs.sgml, but it ought to. I didn't check
> all the other "git grep" matches; please do so.
Oops, I missed many places. I checked all "git grep" matches and fixed them.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 7f4f79d1b5..d52aebaf66 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7750,6 +7750,16 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>subskiplsn</structfield> <type>pg_lsn</type>
+ </para>
+ <para>
+ Finish LSN of the transaction whose changes are to be skipped, if a valid
+ LSN; otherwise <literal>0/0</literal>.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>subname</structfield> <type>name</type>
@@ -7820,16 +7830,6 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</para></entry>
</row>
- <row>
- <entry role="catalog_table_entry"><para role="column_definition">
- <structfield>subskiplsn</structfield> <type>pg_lsn</type>
- </para>
- <para>
- Finish LSN of the transaction whose changes are to be skipped, if a valid
- LSN; otherwise <literal>0/0</literal>.
- </para></entry>
- </row>
-
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>subconninfo</structfield> <type>text</type>
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 0ff0982f7b..add51caadf 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -63,6 +63,7 @@ GetSubscription(Oid subid, bool missing_ok)
sub = (Subscription *) palloc(sizeof(Subscription));
sub->oid = subid;
sub->dbid = subform->subdbid;
+ sub->skiplsn = subform->subskiplsn;
sub->name = pstrdup(NameStr(subform->subname));
sub->owner = subform->subowner;
sub->enabled = subform->subenabled;
@@ -70,7 +71,6 @@ GetSubscription(Oid subid, bool missing_ok)
sub->stream = subform->substream;
sub->twophasestate = subform->subtwophasestate;
sub->disableonerr = subform->subdisableonerr;
- sub->skiplsn = subform->subskiplsn;
/* Get conninfo */
datum = SysCacheGetAttr(SUBSCRIPTIONOID,
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 9eaa51df29..5383b7d3d0 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1285,8 +1285,8 @@ REVOKE ALL ON pg_replication_origin_status FROM public;
-- All columns of pg_subscription except subconninfo are publicly readable.
REVOKE ALL ON pg_subscription FROM public;
-GRANT SELECT (oid, subdbid, subname, subowner, subenabled, subbinary,
- substream, subtwophasestate, subdisableonerr, subskiplsn, subslotname,
+GRANT SELECT (oid, subdbid, subname, subskiplsn, subowner, subenabled,
+ subbinary, substream, subtwophasestate, subdisableonerr, subslotname,
subsynccommit, subpublications)
ON pg_subscription TO public;
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 85dacbe93d..f7b62dd94c 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -596,6 +596,7 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
Anum_pg_subscription_oid);
values[Anum_pg_subscription_oid - 1] = ObjectIdGetDatum(subid);
values[Anum_pg_subscription_subdbid - 1] = ObjectIdGetDatum(MyDatabaseId);
+ values[Anum_pg_subscription_subskiplsn - 1] = LSNGetDatum(InvalidXLogRecPtr);
values[Anum_pg_subscription_subname - 1] =
DirectFunctionCall1(namein, CStringGetDatum(stmt->subname));
values[Anum_pg_subscription_subowner - 1] = ObjectIdGetDatum(owner);
@@ -607,7 +608,6 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
LOGICALREP_TWOPHASE_STATE_PENDING :
LOGICALREP_TWOPHASE_STATE_DISABLED);
values[Anum_pg_subscription_subdisableonerr - 1] = BoolGetDatum(opts.disableonerr);
- values[Anum_pg_subscription_subskiplsn - 1] = LSNGetDatum(InvalidXLogRecPtr);
values[Anum_pg_subscription_subconninfo - 1] =
CStringGetTextDatum(conninfo);
if (opts.slot_name)
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index 599c2e4422..f006a92612 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -54,6 +54,10 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId) BKI_SHARED_RELATION BKI_ROW
Oid subdbid BKI_LOOKUP(pg_database); /* Database the
* subscription is in. */
+
+ XLogRecPtr subskiplsn; /* All changes finished at this LSN are
+ * skipped */
+
NameData subname; /* Name of the subscription */
Oid subowner BKI_LOOKUP(pg_authid); /* Owner of the subscription */
@@ -71,9 +75,6 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId) BKI_SHARED_RELATION BKI_ROW
bool subdisableonerr; /* True if a worker error should cause the
* subscription to be disabled */
- XLogRecPtr subskiplsn; /* All changes finished at this LSN are
- * skipped */
-
#ifdef CATALOG_VARLEN /* variable-length fields start here */
/* Connection string to the publisher */
text subconninfo BKI_FORCE_NOT_NULL;
@@ -103,6 +104,8 @@ typedef struct Subscription
Oid oid; /* Oid of the subscription */
Oid dbid; /* Oid of the database which subscription is
* in */
+ XLogRecPtr skiplsn; /* All changes finished at this LSN are
+ * skipped */
char *name; /* Name of the subscription */
Oid owner; /* Oid of the subscription owner */
bool enabled; /* Indicates if the subscription is enabled */
@@ -113,8 +116,6 @@ typedef struct Subscription
bool disableonerr; /* Indicates if the subscription should be
* automatically disabled if a worker error
* occurs */
- XLogRecPtr skiplsn; /* All changes finished at this LSN are
- * skipped */
char *conninfo; /* Connection string to the publisher */
char *slotname; /* Name of the replication slot */
char *synccommit; /* Synchronous commit setting for worker */
diff --git a/src/test/regress/expected/sanity_check.out b/src/test/regress/expected/sanity_check.out
index 8370c1561c..a2faefb4c0 100644
--- a/src/test/regress/expected/sanity_check.out
+++ b/src/test/regress/expected/sanity_check.out
@@ -25,3 +25,32 @@ SELECT relname, relkind
---------+---------
(0 rows)
+--
+-- When ALIGNOF_DOUBLE==4 (e.g. AIX), the C ABI may impose 8-byte alignment on
+-- some of the C types that correspond to TYPALIGN_DOUBLE SQL types. To ensure
+-- catalog C struct layout matches catalog tuple layout, arrange for the tuple
+-- offset of each fixed-width, attalign='d' catalog column to be divisible by 8
+-- unconditionally. Keep such columns before the first NameData column of the
+-- catalog, since packagers can override NAMEDATALEN to an odd number.
+--
+WITH check_columns AS (
+ SELECT relname, attname,
+ array(
+ SELECT t.oid
+ FROM pg_type t JOIN pg_attribute pa ON t.oid = pa.atttypid
+ WHERE pa.attrelid = a.attrelid AND
+ pa.attnum > 0 AND pa.attnum <= a.attnum
+ ORDER BY pa.attnum) AS coltypes
+ FROM pg_attribute a JOIN pg_class c ON c.oid = attrelid
+ JOIN pg_namespace n ON c.relnamespace = n.oid
+ WHERE attalign = 'd' AND relkind = 'r' AND
+ attnotnull AND attlen <> -1 AND n.nspname = 'pg_catalog'
+)
+SELECT relname, attname, coltypes, get_column_offset(coltypes)
+ FROM check_columns
+ WHERE get_column_offset(coltypes) % 8 != 0 OR
+ 'name'::regtype::oid = ANY(coltypes);
+ relname | attname | coltypes | get_column_offset
+---------+---------+----------+-------------------
+(0 rows)
+
diff --git a/src/test/regress/expected/test_setup.out b/src/test/regress/expected/test_setup.out
index a9d0de3dea..d367c87cfa 100644
--- a/src/test/regress/expected/test_setup.out
+++ b/src/test/regress/expected/test_setup.out
@@ -206,6 +206,10 @@ CREATE FUNCTION ttdummy ()
RETURNS trigger
AS :'regresslib'
LANGUAGE C;
+CREATE FUNCTION get_column_offset (oid[])
+ RETURNS int
+ AS :'regresslib'
+ LANGUAGE C STRICT STABLE;
-- Use hand-rolled hash functions and operator classes to get predictable
-- result on different machines. The hash function for int4 simply returns
-- the sum of the values passed to it and the one for text returns the length
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 0802fb9136..8b0c2d9d68 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -41,6 +41,7 @@
#include "storage/spin.h"
#include "utils/builtins.h"
#include "utils/geo_decls.h"
+#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/rel.h"
#include "utils/typcache.h"
@@ -1216,3 +1217,47 @@ binary_coercible(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(IsBinaryCoercible(srctype, targettype));
}
+
+/*
+ * Return the column offset of the last data in the given array of
+ * data types. The input data types must be fixed-length data types.
+ */
+PG_FUNCTION_INFO_V1(get_column_offset);
+Datum
+get_column_offset(PG_FUNCTION_ARGS)
+{
+ ArrayType *ta = PG_GETARG_ARRAYTYPE_P(0);
+ Oid *type_oids;
+ int ntypes;
+ int column_offset = 0;
+
+ if (ARR_HASNULL(ta) && array_contains_nulls(ta))
+ elog(ERROR, "argument must not contain nulls");
+
+ if (ARR_NDIM(ta) > 1)
+ elog(ERROR, "argument must be empty or one-dimensional array");
+
+ type_oids = (Oid *) ARR_DATA_PTR(ta);
+ ntypes = ArrayGetNItems(ARR_NDIM(ta), ARR_DIMS(ta));
+ for (int i = 0; i < ntypes; i++)
+ {
+ Oid typeoid = type_oids[i];
+ int16 typlen;
+ bool typbyval;
+ char typalign;
+
+ get_typlenbyvalalign(typeoid, &typlen, &typbyval, &typalign);
+
+ /* the data type must be fixed-length */
+ if (!(typbyval || (typlen > 0)))
+ elog(ERROR, "type %u is not fixed-length data type", typeoid);
+
+ column_offset = att_align_nominal(column_offset, typalign);
+
+ /* not include the last type size */
+ if (i != (ntypes - 1))
+ column_offset += typlen;
+ }
+
+ PG_RETURN_INT32(column_offset);
+}
diff --git a/src/test/regress/sql/sanity_check.sql b/src/test/regress/sql/sanity_check.sql
index 162e5324b5..c70ff781fa 100644
--- a/src/test/regress/sql/sanity_check.sql
+++ b/src/test/regress/sql/sanity_check.sql
@@ -19,3 +19,29 @@ SELECT relname, relkind
FROM pg_class
WHERE relkind IN ('v', 'c', 'f', 'p', 'I')
AND relfilenode <> 0;
+
+--
+-- When ALIGNOF_DOUBLE==4 (e.g. AIX), the C ABI may impose 8-byte alignment on
+-- some of the C types that correspond to TYPALIGN_DOUBLE SQL types. To ensure
+-- catalog C struct layout matches catalog tuple layout, arrange for the tuple
+-- offset of each fixed-width, attalign='d' catalog column to be divisible by 8
+-- unconditionally. Keep such columns before the first NameData column of the
+-- catalog, since packagers can override NAMEDATALEN to an odd number.
+--
+WITH check_columns AS (
+ SELECT relname, attname,
+ array(
+ SELECT t.oid
+ FROM pg_type t JOIN pg_attribute pa ON t.oid = pa.atttypid
+ WHERE pa.attrelid = a.attrelid AND
+ pa.attnum > 0 AND pa.attnum <= a.attnum
+ ORDER BY pa.attnum) AS coltypes
+ FROM pg_attribute a JOIN pg_class c ON c.oid = attrelid
+ JOIN pg_namespace n ON c.relnamespace = n.oid
+ WHERE attalign = 'd' AND relkind = 'r' AND
+ attnotnull AND attlen <> -1 AND n.nspname = 'pg_catalog'
+)
+SELECT relname, attname, coltypes, get_column_offset(coltypes)
+ FROM check_columns
+ WHERE get_column_offset(coltypes) % 8 != 0 OR
+ 'name'::regtype::oid = ANY(coltypes);
diff --git a/src/test/regress/sql/test_setup.sql b/src/test/regress/sql/test_setup.sql
index 1f3f2f1724..9c9c8c54b9 100644
--- a/src/test/regress/sql/test_setup.sql
+++ b/src/test/regress/sql/test_setup.sql
@@ -253,6 +253,11 @@ CREATE FUNCTION ttdummy ()
AS :'regresslib'
LANGUAGE C;
+CREATE FUNCTION get_column_offset (oid[])
+ RETURNS int
+ AS :'regresslib'
+ LANGUAGE C STRICT STABLE;
+
-- Use hand-rolled hash functions and operator classes to get predictable
-- result on different machines. The hash function for int4 simply returns
-- the sum of the values passed to it and the one for text returns the length
