public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH 3/5] Remove entries that haven't been used for a certain time
37+ messages / 6 participants
[nested] [flat]

* [PATCH 3/5] Remove entries that haven't been used for a certain time
@ 2018-10-16 04:04  Kyotaro Horiguchi <[email protected]>
  0 siblings, 0 replies; 37+ messages in thread

From: Kyotaro Horiguchi @ 2018-10-16 04:04 UTC (permalink / raw)

Catcache entries can be left alone for several reasons. It is not
desirable that they eat up memory. With this patch, This adds
consideration of removal of entries that haven't been used for a
certain time before enlarging the hash array.

This also can put a hard limit on the number of catcache entries.
---
 doc/src/sgml/config.sgml                      |  40 ++++
 src/backend/tcop/postgres.c                   |  13 ++
 src/backend/utils/cache/catcache.c            | 283 +++++++++++++++++++++++++-
 src/backend/utils/init/globals.c              |   1 +
 src/backend/utils/init/postinit.c             |  11 +
 src/backend/utils/misc/guc.c                  |  43 ++++
 src/backend/utils/misc/postgresql.conf.sample |   2 +
 src/include/miscadmin.h                       |   1 +
 src/include/utils/catcache.h                  |  50 ++++-
 src/include/utils/timeout.h                   |   1 +
 10 files changed, 436 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07b847a8e9..4749ad61a9 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1661,6 +1661,46 @@ include_dir 'conf.d'
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-catalog-cache-prune-min-age" xreflabel="catalog_cache_prune_min_age">
+      <term><varname>catalog_cache_prune_min_age</varname> (<type>integer</type>)
+      <indexterm>
+       <primary><varname>catalog_cache_prune_min_age</varname> configuration
+       parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Specifies the minimum amount of unused time in seconds at which a
+        system catalog cache entry is removed. -1 indicates that this feature
+        is disabled at all. The value defaults to 300 seconds (<literal>5
+        minutes</literal>). The catalog cache entries that are not used for
+        the duration can be removed to prevent it from being filled up with
+        useless entries. This behaviour is muted until the size of a catalog
+        cache exceeds <xref linkend="guc-catalog-cache-memory-target"/>.
+       </para>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry id="guc-catalog-cache-memory-target" xreflabel="catalog_cache_memory_target">
+      <term><varname>catalog_cache_memory_target</varname> (<type>integer</type>)
+      <indexterm>
+       <primary><varname>catalog_cache_memory_target</varname> configuration
+       parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Specifies the maximum amount of memory to which a system catalog cache
+        can expand without pruning in kilobytes. The value defaults to 0,
+        indicating that age-based pruning is always considered. After
+        exceeding this size, catalog cache starts pruning according to
+        <xref linkend="guc-catalog-cache-prune-min-age"/>. If you need to keep
+        certain amount of catalog cache entries with intermittent usage, try
+        increase this setting.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-max-stack-depth" xreflabel="max_stack_depth">
       <term><varname>max_stack_depth</varname> (<type>integer</type>)
       <indexterm>
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 36cfd507b2..f192ee2ca6 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -71,6 +71,7 @@
 #include "tcop/pquery.h"
 #include "tcop/tcopprot.h"
 #include "tcop/utility.h"
+#include "utils/catcache.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/ps_status.h"
@@ -2584,6 +2585,7 @@ start_xact_command(void)
 	 * not desired, the timeout has to be disabled explicitly.
 	 */
 	enable_statement_timeout();
+	SetCatCacheClock(GetCurrentStatementStartTimestamp());
 }
 
 static void
@@ -3159,6 +3161,14 @@ ProcessInterrupts(void)
 
 	if (ParallelMessagePending)
 		HandleParallelMessages();
+
+	if (CatcacheClockTimeoutPending)
+	{
+		CatcacheClockTimeoutPending = 0;
+
+		/* Update timetamp then set up the next timeout */
+		UpdateCatCacheClock();
+	}
 }
 
 
@@ -4021,6 +4031,9 @@ PostgresMain(int argc, char *argv[],
 		QueryCancelPending = false; /* second to avoid race condition */
 		stmt_timeout_active = false;
 
+		/* get sync with the timer state */
+		catcache_clock_timeout_active = false;
+
 		/* Not reading from the client anymore. */
 		DoingCommandRead = false;
 
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 258a1d64cc..04a60a490a 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -39,6 +39,7 @@
 #include "utils/rel.h"
 #include "utils/resowner_private.h"
 #include "utils/syscache.h"
+#include "utils/timeout.h"
 
 
  /* #define CACHEDEBUG */	/* turns DEBUG elogs on */
@@ -71,9 +72,43 @@
 #define CACHE6_elog(a,b,c,d,e,f,g)
 #endif
 
+/* GUC variable to define the minimum age of entries that will be considered to
+ * be evicted in seconds. This variable is shared among various cache
+ * mechanisms.
+ */
+int catalog_cache_prune_min_age = 300;
+
+/*
+ * GUC variable to define the minimum size of hash to cosider entry eviction.
+ * This variable is shared among various cache mechanisms.
+ */
+int catalog_cache_memory_target = 0;
+
+/*
+ * GUC for limit by the number of entries. Entries are removed when the number
+ * of them goes above catalog_cache_entry_limit and leaving newer entries by
+ * the ratio specified by catalog_cache_prune_ratio.
+ */
+int catalog_cache_entry_limit = 0;
+double catalog_cache_prune_ratio = 0.8;
+
+/*
+ * Flag to keep track of whether catcache clock timer is active.
+ */
+bool catcache_clock_timeout_active = false;
+
+/*
+ * Minimum interval between two success move of a cache entry in LRU list,
+ * in microseconds.
+ */
+#define MIN_LRU_UPDATE_INTERVAL 100000	/* 100ms */
+
 /* Cache management header --- pointer is NULL until created */
 static CatCacheHeader *CacheHdr = NULL;
 
+/* Clock used to record the last accessed time of a catcache record. */
+TimestampTz	catcacheclock = 0;
+
 static inline HeapTuple SearchCatCacheInternal(CatCache *cache,
 					   int nkeys,
 					   Datum v1, Datum v2,
@@ -481,6 +516,7 @@ CatCacheRemoveCTup(CatCache *cache, CatCTup *ct)
 
 	/* delink from linked list */
 	dlist_delete(&ct->cache_elem);
+	dlist_delete(&ct->lru_node);
 
 	/*
 	 * Free keys when we're dealing with a negative entry, normal entries just
@@ -490,6 +526,7 @@ CatCacheRemoveCTup(CatCache *cache, CatCTup *ct)
 		CatCacheFreeKeys(cache->cc_tupdesc, cache->cc_nkeys,
 						 cache->cc_keyno, ct->keys);
 
+	cache->cc_memusage -= ct->size;
 	pfree(ct);
 
 	--cache->cc_ntup;
@@ -779,6 +816,7 @@ InitCatCache(int id,
 	MemoryContext oldcxt;
 	size_t		sz;
 	int			i;
+	uint64		base_size;
 
 	/*
 	 * nbuckets is the initial number of hash buckets to use in this catcache.
@@ -821,8 +859,12 @@ InitCatCache(int id,
 	 *
 	 * Note: we rely on zeroing to initialize all the dlist headers correctly
 	 */
+	base_size = MemoryContextGetConsumption(CacheMemoryContext);
 	sz = sizeof(CatCache) + PG_CACHE_LINE_SIZE;
 	cp = (CatCache *) CACHELINEALIGN(palloc0(sz));
+	cp->cc_head_alloc_size =
+		MemoryContextGetConsumption(CacheMemoryContext) - base_size;
+
 	cp->cc_bucket = palloc0(nbuckets * sizeof(dlist_head));
 
 	/*
@@ -842,6 +884,11 @@ InitCatCache(int id,
 	for (i = 0; i < nkeys; ++i)
 		cp->cc_keyno[i] = key[i];
 
+	/* cc_head_alloc_size + consumed size for cc_bucket */
+	cp->cc_memusage =
+		MemoryContextGetConsumption(CacheMemoryContext) - base_size;
+
+	dlist_init(&cp->cc_lru_list);
 	/*
 	 * new cache is initialized as far as we can go for now. print some
 	 * debugging information, if appropriate.
@@ -858,9 +905,185 @@ InitCatCache(int id,
 	 */
 	MemoryContextSwitchTo(oldcxt);
 
+	/* initialize catcache reference clock if haven't done yet */
+	if (catcacheclock == 0)
+		catcacheclock = GetCurrentTimestamp();
+
 	return cp;
 }
 
+/*
+ * helper routine for SetCatCacheClock and UpdateCatCacheClockTimer.
+ *
+ * We need to maintain the catcache clock during a long query.
+ */
+void
+SetupCatCacheClockTimer(void)
+{
+	long delay;
+
+	/* stop timer if not needed */
+	if (catalog_cache_prune_min_age == 0)
+	{
+		catcache_clock_timeout_active = false;
+		return;
+	}
+
+	/* One 10th of the variable, in milliseconds */
+	delay  = catalog_cache_prune_min_age * 1000/10;
+
+	/* Lower limit is 1 second */
+	if (delay < 1000)
+		delay = 1000;
+
+	enable_timeout_after(CATCACHE_CLOCK_TIMEOUT, delay);
+
+	catcache_clock_timeout_active = true;
+}
+
+/*
+ * Update catcacheclock: this is intended to be called from
+ * CATCACHE_CLOCK_TIMEOUT. The interval is expected more than 1 second (see
+ * above), so GetCurrentTime() doesn't harm.
+ */
+void
+UpdateCatCacheClock(void)
+{
+	catcacheclock = GetCurrentTimestamp();
+	SetupCatCacheClockTimer();
+}
+
+/*
+ * It may take an unexpectedly long time before the next clock update when
+ * catalog_cache_prune_min_age gets shorter. Disabling the current timer let
+ * the next update happen at the expected interval. We don't necessariry
+ * require this for increase the age but we don't need to avoid to disable
+ * either.
+ */
+void
+assign_catalog_cache_prune_min_age(int newval, void *extra)
+{
+	if (catcache_clock_timeout_active)
+		disable_timeout(CATCACHE_CLOCK_TIMEOUT, false);
+
+	catcache_clock_timeout_active = false;
+}
+
+/*
+ * CatCacheCleanupOldEntries - Remove infrequently-used entries
+ *
+ * Catcache entries can be left alone for several reasons. We remove them if
+ * they are not accessed for a certain time to prevent catcache from
+ * bloating. The eviction is performed with the similar algorithm with buffer
+ * eviction using access counter. Entries that are accessed several times can
+ * live longer than those that have had less access in the same duration.
+ */
+static bool
+CatCacheCleanupOldEntries(CatCache *cp)
+{
+	int			nremoved = 0;
+	int			nelems_before = cp->cc_ntup;
+	int			ndelelems = 0;
+	bool		prune_by_age = false;
+	bool		prune_by_number = false;
+	dlist_mutable_iter	iter;
+
+	/* prune only if the size of the hash is above the target */
+	if (catalog_cache_prune_min_age >= 0 &&
+		cp->cc_memusage > (Size) catalog_cache_memory_target * 1024L)
+		prune_by_age = true;
+
+	if (catalog_cache_entry_limit > 0 &&
+		nelems_before >= catalog_cache_entry_limit)
+	{
+		ndelelems = nelems_before -
+			(int) (catalog_cache_entry_limit * catalog_cache_prune_ratio);
+
+		/* an arbitrary lower limit.. */
+		if (ndelelems < 256)
+			ndelelems = 256;
+		if (ndelelems > nelems_before)
+			ndelelems = nelems_before;
+
+		prune_by_number = true;
+	}
+
+	/* Return immediately if no pruning is wanted */
+	if (!prune_by_age && !prune_by_number)
+		return false;
+
+	/* Scan over LRU to find entries to remove */
+	dlist_foreach_modify(iter, &cp->cc_lru_list)
+	{
+		CatCTup    *ct = dlist_container(CatCTup, lru_node, iter.cur);
+		bool		remove_this = false;
+
+		/* We don't remove referenced entry */
+		if (ct->refcount != 0 ||
+			(ct->c_list && ct->c_list->refcount != 0))
+			continue;
+
+		/* check against age */
+		if (prune_by_age)
+		{
+			long	entry_age;
+			int		us;
+
+			/*
+			 * Calculate the duration from the time of the last access to the
+			 * "current" time. Since catcacheclock is not advanced within a
+			 * transaction, the entries that are accessed within the current
+			 * transaction won't be pruned.
+			 */
+			TimestampDifference(ct->lastaccess, catcacheclock, &entry_age, &us);
+
+			if (entry_age < catalog_cache_prune_min_age)
+			{
+				/* no longer have a business with further entries, exit  */
+				prune_by_age = false;
+				break;
+			}
+			/*
+			 * Entries that are not accessed after last pruning are removed in
+			 * that seconds, and that has been accessed several times are
+			 * removed after leaving alone for up to three times of the
+			 * duration. We don't try shrink buckets since pruning effectively
+			 * caps catcache expansion in the long term.
+			 */
+			if (ct->naccess > 0)
+				ct->naccess--;
+			else 
+				remove_this = true;
+		}
+
+		/* check against entry number */
+		if (prune_by_number)
+		{
+			if (nremoved < ndelelems)
+				remove_this = true;
+			else
+				prune_by_number = false; /* we're satisfied */
+		}
+
+		/* exit immediately if all finished */
+		if (!prune_by_age && !prune_by_number)
+			break;
+
+		/* do the work */
+		if (remove_this)
+		{
+			CatCacheRemoveCTup(cp, ct);
+			nremoved++;
+		}
+	}
+
+	if (nremoved > 0)
+		elog(DEBUG1, "pruning catalog cache id=%d for %s: removed %d / %d",
+			 cp->id, cp->cc_relname, nremoved, nelems_before);
+
+	return nremoved > 0;
+}
+
 /*
  * Enlarge a catcache, doubling the number of buckets.
  */
@@ -870,6 +1093,7 @@ RehashCatCache(CatCache *cp)
 	dlist_head *newbucket;
 	int			newnbuckets;
 	int			i;
+	uint64		base_size = MemoryContextGetConsumption(CacheMemoryContext);
 
 	elog(DEBUG1, "rehashing catalog cache id %d for %s; %d tups, %d buckets",
 		 cp->id, cp->cc_relname, cp->cc_ntup, cp->cc_nbuckets);
@@ -878,6 +1102,10 @@ RehashCatCache(CatCache *cp)
 	newnbuckets = cp->cc_nbuckets * 2;
 	newbucket = (dlist_head *) MemoryContextAllocZero(CacheMemoryContext, newnbuckets * sizeof(dlist_head));
 
+	/* recalculate memory usage from the first */
+	cp->cc_memusage = cp->cc_head_alloc_size +
+		MemoryContextGetConsumption(CacheMemoryContext) - base_size;
+
 	/* Move all entries from old hash table to new. */
 	for (i = 0; i < cp->cc_nbuckets; i++)
 	{
@@ -890,6 +1118,7 @@ RehashCatCache(CatCache *cp)
 
 			dlist_delete(iter.cur);
 			dlist_push_head(&newbucket[hashIndex], &ct->cache_elem);
+			cp->cc_memusage += ct->size;
 		}
 	}
 
@@ -1274,6 +1503,21 @@ SearchCatCacheInternal(CatCache *cache,
 		 */
 		dlist_move_head(bucket, &ct->cache_elem);
 
+		/* Update access information for pruning */
+		if (ct->naccess < 2)
+			ct->naccess++;
+
+		/*
+		 * We don't want too frequent update of
+		 * LRU. catalog_cache_prune_min_age can be changed on-session so we
+		 * need to maintain the LRU regardless of catalog_cache_prune_min_age.
+		 */
+		if (catcacheclock - ct->lastaccess > MIN_LRU_UPDATE_INTERVAL)
+		{
+			ct->lastaccess = catcacheclock;
+			dlist_move_tail(&cache->cc_lru_list, &ct->lru_node);
+		}
+
 		/*
 		 * If it's a positive entry, bump its refcount and return it. If it's
 		 * negative, we can report failure to the caller.
@@ -1709,6 +1953,11 @@ SearchCatCacheList(CatCache *cache,
 		/* Now we can build the CatCList entry. */
 		oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
 		nmembers = list_length(ctlist);
+
+		/*
+		 * Don't waste a time by counting the list in catcache memory usage,
+		 * since it doesn't live a long life.
+		 */
 		cl = (CatCList *)
 			palloc(offsetof(CatCList, members) + nmembers * sizeof(CatCTup *));
 
@@ -1819,11 +2068,13 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, Datum *arguments,
 	CatCTup    *ct;
 	HeapTuple	dtp;
 	MemoryContext oldcxt;
+	uint64		base_size = MemoryContextGetConsumption(CacheMemoryContext);
 
 	/* negative entries have no tuple associated */
 	if (ntp)
 	{
 		int			i;
+		int			tupsize;
 
 		Assert(!negative);
 
@@ -1842,8 +2093,8 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, Datum *arguments,
 		/* Allocate memory for CatCTup and the cached tuple in one go */
 		oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
 
-		ct = (CatCTup *) palloc(sizeof(CatCTup) +
-								MAXIMUM_ALIGNOF + dtp->t_len);
+		tupsize = sizeof(CatCTup) +	MAXIMUM_ALIGNOF + dtp->t_len;
+		ct = (CatCTup *) palloc(tupsize);
 		ct->tuple.t_len = dtp->t_len;
 		ct->tuple.t_self = dtp->t_self;
 		ct->tuple.t_tableOid = dtp->t_tableOid;
@@ -1877,7 +2128,6 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, Datum *arguments,
 		Assert(negative);
 		oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
 		ct = (CatCTup *) palloc(sizeof(CatCTup));
-
 		/*
 		 * Store keys - they'll point into separately allocated memory if not
 		 * by-value.
@@ -1898,18 +2148,36 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, Datum *arguments,
 	ct->dead = false;
 	ct->negative = negative;
 	ct->hash_value = hashValue;
+	ct->naccess = 0;
+	ct->lastaccess = catcacheclock;
+	dlist_push_tail(&cache->cc_lru_list, &ct->lru_node);
 
 	dlist_push_head(&cache->cc_bucket[hashIndex], &ct->cache_elem);
 
 	cache->cc_ntup++;
 	CacheHdr->ch_ntup++;
 
+	ct->size = MemoryContextGetConsumption(CacheMemoryContext) - base_size;
+	cache->cc_memusage += ct->size;
+
+	/* increase refcount so that this survives pruning */
+	ct->refcount++;
+
 	/*
-	 * If the hash table has become too full, enlarge the buckets array. Quite
-	 * arbitrarily, we enlarge when fill factor > 2.
+	 * If the hash table has become too full, try cleanup by removing
+	 * infrequently used entries to make a room for the new entry. If it
+	 * failed, enlarge the bucket array instead.  Quite arbitrarily, we try
+	 * this when fill factor > 2.
 	 */
-	if (cache->cc_ntup > cache->cc_nbuckets * 2)
+	if (cache->cc_ntup > cache->cc_nbuckets * 2 &&
+		!CatCacheCleanupOldEntries(cache))
 		RehashCatCache(cache);
+	/* we may still want to prune by entry number, check it */
+	else if (catalog_cache_entry_limit > 0 &&
+			 cache->cc_ntup > catalog_cache_entry_limit)
+		CatCacheCleanupOldEntries(cache);
+
+	ct->refcount--;
 
 	return ct;
 }
@@ -1940,7 +2208,7 @@ CatCacheFreeKeys(TupleDesc tupdesc, int nkeys, int *attnos, Datum *keys)
 /*
  * Helper routine that copies the keys in the srckeys array into the dstkeys
  * one, guaranteeing that the datums are fully allocated in the current memory
- * context.
+ * context. Returns allocated memory size.
  */
 static void
 CatCacheCopyKeys(TupleDesc tupdesc, int nkeys, int *attnos,
@@ -1976,7 +2244,6 @@ CatCacheCopyKeys(TupleDesc tupdesc, int nkeys, int *attnos,
 							   att->attbyval,
 							   att->attlen);
 	}
-
 }
 
 /*
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index fd51934aaf..0e8b972a29 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -32,6 +32,7 @@ volatile sig_atomic_t QueryCancelPending = false;
 volatile sig_atomic_t ProcDiePending = false;
 volatile sig_atomic_t ClientConnectionLost = false;
 volatile sig_atomic_t IdleInTransactionSessionTimeoutPending = false;
+volatile sig_atomic_t CatcacheClockTimeoutPending = false;
 volatile sig_atomic_t ConfigReloadPending = false;
 volatile uint32 InterruptHoldoffCount = 0;
 volatile uint32 QueryCancelHoldoffCount = 0;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index a5ee209f91..9eb50e9676 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -72,6 +72,7 @@ static void ShutdownPostgres(int code, Datum arg);
 static void StatementTimeoutHandler(void);
 static void LockTimeoutHandler(void);
 static void IdleInTransactionSessionTimeoutHandler(void);
+static void CatcacheClockTimeoutHandler(void);
 static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
@@ -628,6 +629,8 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 		RegisterTimeout(LOCK_TIMEOUT, LockTimeoutHandler);
 		RegisterTimeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
 						IdleInTransactionSessionTimeoutHandler);
+		RegisterTimeout(CATCACHE_CLOCK_TIMEOUT,
+						CatcacheClockTimeoutHandler);
 	}
 
 	/*
@@ -1238,6 +1241,14 @@ IdleInTransactionSessionTimeoutHandler(void)
 	SetLatch(MyLatch);
 }
 
+static void
+CatcacheClockTimeoutHandler(void)
+{
+	CatcacheClockTimeoutPending = true;
+	InterruptPending = true;
+	SetLatch(MyLatch);
+}
+
 /*
  * Returns true if at least one role is defined in this database cluster.
  */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 41d477165c..c62d5ad8b8 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -81,6 +81,7 @@
 #include "tsearch/ts_cache.h"
 #include "utils/builtins.h"
 #include "utils/bytea.h"
+#include "utils/catcache.h"
 #include "utils/guc_tables.h"
 #include "utils/float.h"
 #include "utils/memutils.h"
@@ -2205,6 +2206,38 @@ static struct config_int ConfigureNamesInt[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"catalog_cache_prune_min_age", PGC_USERSET, RESOURCES_MEM,
+			gettext_noop("Sets the minimum unused duration of cache entries before removal."),
+			gettext_noop("Catalog cache entries that live unused for longer than this seconds are considered to be removed."),
+			GUC_UNIT_S
+		},
+		&catalog_cache_prune_min_age,
+		300, -1, INT_MAX,
+		NULL, assign_catalog_cache_prune_min_age, NULL
+	},
+
+	{
+		{"catalog_cache_memory_target", PGC_USERSET, RESOURCES_MEM,
+			gettext_noop("Sets the minimum syscache size to keep."),
+			gettext_noop("Time-based cache pruning starts working after exceeding this size."),
+			GUC_UNIT_KB
+		},
+		&catalog_cache_memory_target,
+		0, 0, MAX_KILOBYTES,
+		NULL, NULL, NULL
+	},
+
+	{
+		{"catalog_cache_entry_limit", PGC_USERSET, RESOURCES_MEM,
+			gettext_noop("Sets the maximum entries of catcache."),
+		 	NULL
+		},
+		&catalog_cache_entry_limit,
+		0, 0, INT_MAX,
+		NULL, NULL, NULL
+	},
+
 	/*
 	 * We use the hopefully-safely-small value of 100kB as the compiled-in
 	 * default for max_stack_depth.  InitializeGUCOptions will increase it if
@@ -3368,6 +3401,16 @@ static struct config_real ConfigureNamesReal[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"catalog_cache_prune_ratio", PGC_USERSET, RESOURCES_MEM,
+			gettext_noop("Reduce ratio of pruning caused by catalog_cache_entry_limit."),
+		 	NULL
+		},
+		&catalog_cache_prune_ratio,
+		0.8, 0.0, 1.0,
+		NULL, NULL, NULL
+	},
+
 	/* End-of-list marker */
 	{
 		{NULL, 0, 0, NULL, NULL}, NULL, 0.0, 0.0, 0.0, NULL, NULL, NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index ad6c436f93..aeb5968e75 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -128,6 +128,8 @@
 #work_mem = 4MB				# min 64kB
 #maintenance_work_mem = 64MB		# min 1MB
 #autovacuum_work_mem = -1		# min 1MB, or -1 to use maintenance_work_mem
+#catalog_cache_memory_target = 0kB	# in kB
+#catalog_cache_prune_min_age = 300s	# -1 disables pruning
 #max_stack_depth = 2MB			# min 100kB
 #shared_memory_type = mmap		# the default is the first option
 					# supported by the operating system:
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index c9e35003a5..33b800e80f 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -82,6 +82,7 @@ extern PGDLLIMPORT volatile sig_atomic_t InterruptPending;
 extern PGDLLIMPORT volatile sig_atomic_t QueryCancelPending;
 extern PGDLLIMPORT volatile sig_atomic_t ProcDiePending;
 extern PGDLLIMPORT volatile sig_atomic_t IdleInTransactionSessionTimeoutPending;
+extern PGDLLIMPORT volatile sig_atomic_t CatcacheClockTimeoutPending;
 extern PGDLLIMPORT volatile sig_atomic_t ConfigReloadPending;
 
 extern PGDLLIMPORT volatile sig_atomic_t ClientConnectionLost;
diff --git a/src/include/utils/catcache.h b/src/include/utils/catcache.h
index 65d816a583..0a714bf514 100644
--- a/src/include/utils/catcache.h
+++ b/src/include/utils/catcache.h
@@ -22,6 +22,7 @@
 
 #include "access/htup.h"
 #include "access/skey.h"
+#include "datatype/timestamp.h"
 #include "lib/ilist.h"
 #include "utils/relcache.h"
 
@@ -61,6 +62,11 @@ typedef struct catcache
 	slist_node	cc_next;		/* list link */
 	ScanKeyData cc_skey[CATCACHE_MAXKEYS];	/* precomputed key info for heap
 											 * scans */
+	dlist_head	cc_lru_list;
+	int			cc_head_alloc_size;/* consumed memory to allocate this struct */
+	int			cc_memusage;	/* memory usage of this catcache (excluding
+								 * header part) */
+	int			cc_nfreeent;	/* # of entries currently not referenced */
 
 	/*
 	 * Keep these at the end, so that compiling catcache.c with CATCACHE_STATS
@@ -119,7 +125,10 @@ typedef struct catctup
 	bool		dead;			/* dead but not yet removed? */
 	bool		negative;		/* negative cache entry? */
 	HeapTupleData tuple;		/* tuple management header */
-
+	int			naccess;		/* # of access to this entry, up to 2  */
+	TimestampTz	lastaccess;		/* approx. timestamp of the last usage */
+	dlist_node	lru_node;		/* LRU node */
+	int			size;			/* palloc'ed size off this tuple */
 	/*
 	 * The tuple may also be a member of at most one CatCList.  (If a single
 	 * catcache is list-searched with varying numbers of keys, we may have to
@@ -189,6 +198,45 @@ typedef struct catcacheheader
 /* this extern duplicates utils/memutils.h... */
 extern PGDLLIMPORT MemoryContext CacheMemoryContext;
 
+/* for guc.c, not PGDLLPMPORT'ed */
+extern int catalog_cache_prune_min_age;
+extern int catalog_cache_memory_target;
+extern int catalog_cache_entry_limit;
+extern double catalog_cache_prune_ratio;
+
+/* to use as access timestamp of catcache entries */
+extern TimestampTz catcacheclock;
+
+/*
+ * Flag to keep track of whether catcache timestamp timer is active.
+ */
+extern bool catcache_clock_timeout_active;
+
+/* catcache prune time helper functions  */
+extern void SetupCatCacheClockTimer(void);
+extern void UpdateCatCacheClock(void);
+
+/*
+ * SetCatCacheClock - set timestamp for catcache access record and start
+ * maintenance timer if needed. We keep to update the clock even while pruning
+ * is disable so that we are not confused by bogus clock value.
+ */
+static inline void
+SetCatCacheClock(TimestampTz ts)
+{
+	catcacheclock = ts;
+
+	if (!catcache_clock_timeout_active && catalog_cache_prune_min_age > 0)
+		SetupCatCacheClockTimer();
+}
+
+static inline TimestampTz
+GetCatCacheClock(void)
+{
+	return catcacheclock;
+}
+
+extern void assign_catalog_cache_prune_min_age(int newval, void *extra);
 extern void CreateCacheMemoryContext(void);
 
 extern CatCache *InitCatCache(int id, Oid reloid, Oid indexoid,
diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h
index 9244a2a7b7..b2d97b4f7b 100644
--- a/src/include/utils/timeout.h
+++ b/src/include/utils/timeout.h
@@ -31,6 +31,7 @@ typedef enum TimeoutId
 	STANDBY_TIMEOUT,
 	STANDBY_LOCK_TIMEOUT,
 	IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
+	CATCACHE_CLOCK_TIMEOUT,
 	/* First user-definable timeout reason */
 	USER_TIMEOUT,
 	/* Maximum number of timeout reasons */
-- 
2.16.3


----Next_Part(Wed_Feb_13_15_31_14_2019_467)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="v15-0004-Syscache-usage-tracking-feature.patch"



^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-04-07 17:42  Tom Lane <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Tom Lane @ 2022-04-07 17:42 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Niyas Sait <[email protected]>; PostgreSQL Hackers <[email protected]>

Andres Freund <[email protected]> writes:
> On 2022-04-07 09:40:43 -0400, Tom Lane wrote:
>> If it causes problems down the road, how will we debug it?

> If what causes problems down the road? Afaics the patch doesn't change
> anything outside of windows-on-arm, so it shouldn't cause any breakage we care
> about until we get a buildfarm animal.

Do we have a volunteer to run a buildfarm animal?  I don't see much
point in thinking about this till there is one ready to go.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-04-07 17:53  Andres Freund <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Andres Freund @ 2022-04-07 17:53 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; Niyas Sait <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Hi,

On 2022-04-07 13:42:49 -0400, Tom Lane wrote:
> Andres Freund <[email protected]> writes:
> > On 2022-04-07 09:40:43 -0400, Tom Lane wrote:
> >> If it causes problems down the road, how will we debug it?
>
> > If what causes problems down the road? Afaics the patch doesn't change
> > anything outside of windows-on-arm, so it shouldn't cause any breakage we care
> > about until we get a buildfarm animal.
>
> Do we have a volunteer to run a buildfarm animal?  I don't see much
> point in thinking about this till there is one ready to go.

OP said that they might be able to:
https://www.postgresql.org/message-id/CAFPTBD_NZb%3Dq_6uE-QV%2BS%2Bpm%3DRc9sBKrg8CQ_Y3dc27Awrm7cQ%40...

Niyas, any updates on that?

Greetings,

Andres Freund






^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-04-19 14:22  Niyas Sait <[email protected]>
  parent: Andres Freund <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Niyas Sait @ 2022-04-19 14:22 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

 > Niyas, any updates on that?

Sorry for the delay! Configuring the scripts took some time. I have
successfully run the builfarm animal script using my git repository [1]
which contains the proposed patch on a Windows Arm64 machine.

I made a request to add a new machine to build farm through [2].

I believe we should be able to enable the build farm machine once the
change has been merged.

Thanks,
Niyas

[1] https://github.com/nsait-linaro/postgres.git
[2] https://buildfarm.postgresql.org/cgi-bin/register-form.pl


On Thu, 7 Apr 2022 at 18:54, Andres Freund <[email protected]> wrote:

> Hi,
>
> On 2022-04-07 13:42:49 -0400, Tom Lane wrote:
> > Andres Freund <[email protected]> writes:
> > > On 2022-04-07 09:40:43 -0400, Tom Lane wrote:
> > >> If it causes problems down the road, how will we debug it?
> >
> > > If what causes problems down the road? Afaics the patch doesn't change
> > > anything outside of windows-on-arm, so it shouldn't cause any breakage
> we care
> > > about until we get a buildfarm animal.
> >
> > Do we have a volunteer to run a buildfarm animal?  I don't see much
> > point in thinking about this till there is one ready to go.
>
> OP said that they might be able to:
>
> https://www.postgresql.org/message-id/CAFPTBD_NZb%3Dq_6uE-QV%2BS%2Bpm%3DRc9sBKrg8CQ_Y3dc27Awrm7cQ%40...
>
> Niyas, any updates on that?
>
> Greetings,
>
> Andres Freund
>


^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-04-20 00:28  Michael Paquier <[email protected]>
  parent: Niyas Sait <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Michael Paquier @ 2022-04-20 00:28 UTC (permalink / raw)
  To: Niyas Sait <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Tue, Apr 19, 2022 at 03:22:30PM +0100, Niyas Sait wrote:
> Sorry for the delay! Configuring the scripts took some time. I have
> successfully run the builfarm animal script using my git repository [1]
> which contains the proposed patch on a Windows Arm64 machine.
> 
> I made a request to add a new machine to build farm through [2].

Have you tested with the amount of coverage provided by vcregress.pl?

Another thing I was wondering about is if it would be possible to have
this option in Travis, but that does not seem to be the case:
https://docs.travis-ci.com/user/reference/windows/#windows-version

> I believe we should be able to enable the build farm machine once the
> change has been merged.

Better to wait for the beginning of the development cycle at this
stage, based on all the replies received.  That would bring us to the
beginning of July.

+               if ($solution->{platform} eq 'ARM64') {
+                       push(@pgportfiles, 'pg_crc32c_armv8_choose.c');
+                       push(@pgportfiles, 'pg_crc32c_armv8.c');
+               } else {
+                       push(@pgportfiles, 'pg_crc32c_sse42_choose.c');
+                       push(@pgportfiles, 'pg_crc32c_sse42.c');
+               }

+++ b/src/port/pg_crc32c_armv8.c
+#ifndef _MSC_VER
 #include <arm_acle.h>
+#endif
[ ... ]
+#ifdef _M_ARM64
+   /*
+    * arm64 way of hinting processor for spin loops optimisations
+    * ref: https://community.arm.com/support-forums/f/infrastructure-solutions-forum/48654/ssetoneon-faq
+    */
+   __isb(_ARM64_BARRIER_SY);
+#else
I think that such extra optimizations had better be in a separate
patch, and we should focus on getting the build done first.

+   # arm64 linker only supports dynamic base address
+   my $cfgrandbaseaddress = $self->{platform} eq 'ARM64' ? 'true' : 'false';
This issue is still lying around, and you may have been lucky.  Would
there be any issues to remove this change to get a basic support in?
As mentioned upthread, there is a long history of Postgres with ASLR.

This would mean that a basic patch could be done with just the changes
for gendef.pl, and the first part of the changes inMSBuildProject.pm.
Would that not be enough?
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../Yl9TmT%[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-04-20 09:43  Niyas Sait <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Niyas Sait @ 2022-04-20 09:43 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

> Have you tested with the amount of coverage provided by vcregress.pl?

I built and ran the relevant tests with the help of run_build.pl.

I think following tests are executed - check, contribcheck, ecpgcheck,
installcheck, isolationcheck, modulescheck, and upgradecheck.

> Another thing I was wondering about is if it would be possible to have
> this option in Travis, but that does not seem to be the case:
> https://docs.travis-ci.com/user/reference/windows/#windows-version

Yes, I think Travis doesn't yet support Windows/Arm64 platform.

> +               if ($solution->{platform} eq 'ARM64') {
> +                       push(@pgportfiles, 'pg_crc32c_armv8_choose.c');
> +                       push(@pgportfiles, 'pg_crc32c_armv8.c');
> +               } else {
> +                       push(@pgportfiles, 'pg_crc32c_sse42_choose.c');
> +                       push(@pgportfiles, 'pg_crc32c_sse42.c');
> +               }
>
> +++ b/src/port/pg_crc32c_armv8.c
> +#ifndef _MSC_VER
>  #include <arm_acle.h>
> +#endif
> [ ... ]
> +#ifdef _M_ARM64
> +   /*
> +    * arm64 way of hinting processor for spin loops optimisations
> +    * ref:
https://community.arm.com/support-forums/f/infrastructure-solutions-forum/48654/ssetoneon-faq
> +    */
> +   __isb(_ARM64_BARRIER_SY);
> +#else
> I think that such extra optimizations had better be in a separate
> patch, and we should focus on getting the build done first.

> This would mean that a basic patch could be done with just the changes
> for gendef.pl, and the first part of the changes inMSBuildProject.pm.
> Would that not be enough?

I cannot build without any of the above changes. Nothing specific for
optimization
is added to this patch.

> +   # arm64 linker only supports dynamic base address
> +   my $cfgrandbaseaddress = $self->{platform} eq 'ARM64' ? 'true' :
'false';
> This issue is still lying around, and you may have been lucky.  Would
> there be any issues to remove this change to get a basic support in?
> As mentioned upthread, there is a long history of Postgres with ASLR.

MSVC linker doesn't allow non-random base addresses for Arm64 platforms.
It is needed for basic support.

Niyas


^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-04-21 04:07  Michael Paquier <[email protected]>
  parent: Niyas Sait <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Michael Paquier @ 2022-04-21 04:07 UTC (permalink / raw)
  To: Niyas Sait <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Wed, Apr 20, 2022 at 10:43:06AM +0100, Niyas Sait wrote:
>> This issue is still lying around, and you may have been lucky.  Would
>> there be any issues to remove this change to get a basic support in?
>> As mentioned upthread, there is a long history of Postgres with ASLR.
> 
> MSVC linker doesn't allow non-random base addresses for Arm64 platforms.
> It is needed for basic support.

Why does it not allow that?  Is that just a limitation of the
compiler?  If yes, what is the error happening?  This question is not
exactly answered yet as of this thread.  I may be missing a reference
about that in the upstream docs, but I see nowhere an explanation
about the reason and the why.  That's also one of the first questions
from Thomas upthread.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-04-21 09:21  Niyas Sait <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Niyas Sait @ 2022-04-21 09:21 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

> Why does it not allow that?  Is that just a limitation of the
> compiler?  If yes, what is the error happening?  This question is not
> exactly answered yet as of this thread.  I may be missing a reference
> about that in the upstream docs, but I see nowhere an explanation
> about the reason and the why.  That's also one of the first questions
> from Thomas upthread.

The following error occurs:

LINK : fatal error LNK1246: '/DYNAMICBASE:NO' not compatible with 'ARM64'
target machine; link without '/DYNAMICBASE:NO

This seems to be a deliberate restriction for Arm64 targets. However, no
references were provided. To clarify, I have posted a question [1] on the
community channel of Visual Studio.

Niyas

[1]
https://developercommunity.visualstudio.com/t/LINK-:-fatal-error-LNK1246:-DYNAMICBAS/10020163





On Thu, 21 Apr 2022 at 05:07, Michael Paquier <[email protected]> wrote:

> On Wed, Apr 20, 2022 at 10:43:06AM +0100, Niyas Sait wrote:
> >> This issue is still lying around, and you may have been lucky.  Would
> >> there be any issues to remove this change to get a basic support in?
> >> As mentioned upthread, there is a long history of Postgres with ASLR.
> >
> > MSVC linker doesn't allow non-random base addresses for Arm64 platforms.
> > It is needed for basic support.
>
> Why does it not allow that?  Is that just a limitation of the
> compiler?  If yes, what is the error happening?  This question is not
> exactly answered yet as of this thread.  I may be missing a reference
> about that in the upstream docs, but I see nowhere an explanation
> about the reason and the why.  That's also one of the first questions
> from Thomas upthread.
> --
> Michael
>


^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-04-25 01:17  Michael Paquier <[email protected]>
  parent: Niyas Sait <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Michael Paquier @ 2022-04-25 01:17 UTC (permalink / raw)
  To: Niyas Sait <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Thu, Apr 21, 2022 at 10:21:04AM +0100, Niyas Sait wrote:
> The following error occurs:
> 
> LINK : fatal error LNK1246: '/DYNAMICBASE:NO' not compatible with 'ARM64'
> target machine; link without '/DYNAMICBASE:NO

Okay, that's interesting.  In light of things like 7f3e17b, that may
be annoying.  Perhaps newer Windows versions are able to handle that
better.  I am wondering if it would be worth re-evaluating this
change, and attempt to re-enable that in environments other than
arm64.  This could become interesting if we consider that there have
been talks to cut the support for a bunch of Windows versions to focus
on the newer ones only.

> This seems to be a deliberate restriction for Arm64 targets. However, no
> references were provided. To clarify, I have posted a question [1] on the
> community channel of Visual Studio.

Thanks for doing that!  Your post is just a couple of days old, let's
see if we get a reply on that.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-05-09 11:44  Niyas Sait <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Niyas Sait @ 2022-05-09 11:44 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

> Thanks for doing that!  Your post is just a couple of days old, let's
> see if we get a reply on that.

Microsoft updated documentation [1] and clarified that ASLR cannot be
disabled for Arm64 targets.

Because ASLR can't be disabled on ARM, ARM64, or ARM64EC architectures,
> /DYNAMICBASE:NO isn't supported for these targets.


Thanks
Niyas

[1]
https://docs.microsoft.com/en-us/cpp/build/reference/dynamicbase?view=msvc-170


On Mon, 25 Apr 2022 at 02:17, Michael Paquier <[email protected]> wrote:

> On Thu, Apr 21, 2022 at 10:21:04AM +0100, Niyas Sait wrote:
> > The following error occurs:
> >
> > LINK : fatal error LNK1246: '/DYNAMICBASE:NO' not compatible with 'ARM64'
> > target machine; link without '/DYNAMICBASE:NO
>
> Okay, that's interesting.  In light of things like 7f3e17b, that may
> be annoying.  Perhaps newer Windows versions are able to handle that
> better.  I am wondering if it would be worth re-evaluating this
> change, and attempt to re-enable that in environments other than
> arm64.  This could become interesting if we consider that there have
> been talks to cut the support for a bunch of Windows versions to focus
> on the newer ones only.
>
> > This seems to be a deliberate restriction for Arm64 targets. However, no
> > references were provided. To clarify, I have posted a question [1] on the
> > community channel of Visual Studio.
>
> Thanks for doing that!  Your post is just a couple of days old, let's
> see if we get a reply on that.
> --
> Michael
>


^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-05-10 01:01  Michael Paquier <[email protected]>
  parent: Niyas Sait <[email protected]>
  0 siblings, 2 replies; 37+ messages in thread

From: Michael Paquier @ 2022-05-10 01:01 UTC (permalink / raw)
  To: Niyas Sait <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Mon, May 09, 2022 at 12:44:22PM +0100, Niyas Sait wrote:
> Microsoft updated documentation [1] and clarified that ASLR cannot be
> disabled for Arm64 targets.
> 
> Because ASLR can't be disabled on ARM, ARM64, or ARM64EC architectures,
> > /DYNAMICBASE:NO isn't supported for these targets.

Better than nothing, I guess, though this does not stand as a reason
explaining why this choice has been made.  And it looks like we will
never know.  We are going to need more advanced testing to check that
DYNAMICBASE is stable enough if we begin to enable it.  Perhaps we
should really look at that for all the build targets we currently
support rather than just ARM, for the most modern Win32 environments
if we are going to cut support for most of the oldest releases..
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../Ynm5g2152%2FyKc%[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-07-14 08:36  Niyas Sait <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 0 replies; 37+ messages in thread

From: Niyas Sait @ 2022-07-14 08:36 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Hello,

Please find a new patch (no further changes) rebased on top of the master.

On Tue, 10 May 2022 at 02:02, Michael Paquier <[email protected]> wrote:

> On Mon, May 09, 2022 at 12:44:22PM +0100, Niyas Sait wrote:
> > Microsoft updated documentation [1] and clarified that ASLR cannot be
> > disabled for Arm64 targets.
> >
> > Because ASLR can't be disabled on ARM, ARM64, or ARM64EC architectures,
> > > /DYNAMICBASE:NO isn't supported for these targets.
>
> Better than nothing, I guess, though this does not stand as a reason
> explaining why this choice has been made.  And it looks like we will
> never know.  We are going to need more advanced testing to check that
> DYNAMICBASE is stable enough if we begin to enable it.  Perhaps we
> should really look at that for all the build targets we currently
> support rather than just ARM, for the most modern Win32 environments
> if we are going to cut support for most of the oldest releases..
> --
> Michael
>


-- 
Niyas Sait
Linaro Limited


Attachments:

  [application/octet-stream] v2-0001-Enable-postgres-native-build-for-windows-arm64-pl.patch (5.7K, ../../CAFPTBD_zcrXZihjbyUENiMLkx8JH+FrSEGFr0E0ZX4VKnmoj4w@mail.gmail.com/3-v2-0001-Enable-postgres-native-build-for-windows-arm64-pl.patch)
  download | inline diff:
From 36b5ef714ca4213a25a5f2b02e936826c8dcae43 Mon Sep 17 00:00:00 2001
From: Niyas Sait <[email protected]>
Date: Tue, 22 Feb 2022 13:07:24 +0000
Subject: [PATCH v2] Enable postgres native build for windows-arm64 platform

Following changes are included

- Extend MSVC scripts to handle ARM64 platform
- Add arm64 definition of spin_delay function
- Exclude arm_acle.h import for MSVC
---
 src/include/storage/s_lock.h     | 10 +++++++++-
 src/port/pg_crc32c_armv8.c       |  2 ++
 src/tools/msvc/MSBuildProject.pm | 15 +++++++++++----
 src/tools/msvc/Mkvcbuild.pm      |  9 +++++++--
 src/tools/msvc/Solution.pm       |  9 +++++++--
 src/tools/msvc/gendef.pl         |  8 ++++----
 6 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 1c9f6f0895..5ee3bddabc 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -703,13 +703,21 @@ typedef LONG slock_t;
 #define SPIN_DELAY() spin_delay()
 
 /* If using Visual C++ on Win64, inline assembly is unavailable.
- * Use a _mm_pause intrinsic instead of rep nop.
+ * Use _mm_pause (x64) or __isb(arm64) intrinsic instead of rep nop.
  */
 #if defined(_WIN64)
 static __forceinline void
 spin_delay(void)
 {
+#ifdef _M_ARM64
+	/*
+	 * arm64 way of hinting processor for spin loops optimisations
+	 * ref: https://community.arm.com/support-forums/f/infrastructure-solutions-forum/48654/ssetoneon-faq
+	*/
+	__isb(_ARM64_BARRIER_SY);
+#else
 	_mm_pause();
+#endif
 }
 #else
 static __forceinline void
diff --git a/src/port/pg_crc32c_armv8.c b/src/port/pg_crc32c_armv8.c
index 9e301f96f6..981718752f 100644
--- a/src/port/pg_crc32c_armv8.c
+++ b/src/port/pg_crc32c_armv8.c
@@ -14,7 +14,9 @@
  */
 #include "c.h"
 
+#ifndef _MSC_VER
 #include <arm_acle.h>
+#endif
 
 #include "port/pg_crc32c.h"
 
diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
index 62acdda3a1..5128dc9bd6 100644
--- a/src/tools/msvc/MSBuildProject.pm
+++ b/src/tools/msvc/MSBuildProject.pm
@@ -310,11 +310,18 @@ sub WriteItemDefinitionGroup
 	  : ($self->{type} eq "dll" ? 'DynamicLibrary' : 'StaticLibrary');
 	my $libs = $self->GetAdditionalLinkerDependencies($cfgname, ';');
 
-	my $targetmachine =
-	  $self->{platform} eq 'Win32' ? 'MachineX86' : 'MachineX64';
+	my $targetmachine;
+	if ($self->{platform} eq 'Win32') {
+		$targetmachine = 'MachineX86';
+	} elsif ($self->{platform} eq 'ARM64'){
+		$targetmachine = 'MachineARM64';
+	} else {
+		$targetmachine = 'MachineX64';
+	}
 
 	my $includes = join ';', @{ $self->{includes} }, "";
-
+	# arm64 linker only supports dynamic base address
+	my $cfgrandbaseaddress = $self->{platform} eq 'ARM64' ? 'true' : 'false';
 	print $f <<EOF;
   <ItemDefinitionGroup Condition="'\$(Configuration)|\$(Platform)'=='$cfgname|$self->{platform}'">
     <ClCompile>
@@ -347,7 +354,7 @@ sub WriteItemDefinitionGroup
       <ProgramDatabaseFile>.\\$cfgname\\$self->{name}\\$self->{name}.pdb</ProgramDatabaseFile>
       <GenerateMapFile>false</GenerateMapFile>
       <MapFileName>.\\$cfgname\\$self->{name}\\$self->{name}.map</MapFileName>
-      <RandomizedBaseAddress>false</RandomizedBaseAddress>
+      <RandomizedBaseAddress>$cfgrandbaseaddress</RandomizedBaseAddress>
       <!-- Permit links to MinGW-built, 32-bit DLLs (default before VS2012). -->
       <ImageHasSafeExceptionHandlers/>
       <SubSystem>Console</SubSystem>
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index e4feda10fd..fab85a957e 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -114,8 +114,13 @@ sub mkvcbuild
 
 	if ($vsVersion >= '9.00')
 	{
-		push(@pgportfiles, 'pg_crc32c_sse42_choose.c');
-		push(@pgportfiles, 'pg_crc32c_sse42.c');
+		if ($solution->{platform} eq 'ARM64') {
+			push(@pgportfiles, 'pg_crc32c_armv8_choose.c');
+			push(@pgportfiles, 'pg_crc32c_armv8.c');
+		} else {
+			push(@pgportfiles, 'pg_crc32c_sse42_choose.c');
+			push(@pgportfiles, 'pg_crc32c_sse42.c');
+		}
 		push(@pgportfiles, 'pg_crc32c_sb8.c');
 	}
 	else
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index fa32dc371d..6d4ba86167 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -67,8 +67,13 @@ sub DeterminePlatform
 		# Examine CL help output to determine if we are in 32 or 64-bit mode.
 		my $output = `cl /help 2>&1`;
 		$? >> 8 == 0 or die "cl command not found";
-		$self->{platform} =
-		  ($output =~ /^\/favor:<.+AMD64/m) ? 'x64' : 'Win32';
+		if ($output =~ /^\/favor:<.+AMD64/m) {
+			$self->{platform} = 'x64';
+		} elsif($output =~ /for ARM64$/m) {
+			$self->{platform} = 'ARM64';
+		} else {
+			$self->{platform} = 'Win32';
+		}
 	}
 	else
 	{
diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index b8c514a831..2068484b14 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -120,9 +120,9 @@ sub writedef
 	{
 		my $isdata = $def->{$f} eq 'data';
 
-		# Strip the leading underscore for win32, but not x64
+		# Strip the leading underscore for win32, but not x64 and arm64
 		$f =~ s/^_//
-		  unless ($platform eq "x64");
+		  unless ($platform ne "Win32");
 
 		# Emit just the name if it's a function symbol, or emit the name
 		# decorated with the DATA option for variables.
@@ -144,13 +144,13 @@ sub usage
 {
 	die(    "Usage: gendef.pl <modulepath> <platform>\n"
 		  . "    modulepath: path to dir with obj files, no trailing slash"
-		  . "    platform: Win32 | x64");
+		  . "    platform: Win32 | x64 | ARM64");
 }
 
 usage()
   unless scalar(@ARGV) == 2
   && ( ($ARGV[0] =~ /\\([^\\]+$)/)
-	&& ($ARGV[1] eq 'Win32' || $ARGV[1] eq 'x64'));
+	&& ($ARGV[1] eq 'Win32' || $ARGV[1] eq 'x64' || $ARGV[1] eq 'ARM64'));
 my $defname  = uc $1;
 my $deffile  = "$ARGV[0]/$defname.def";
 my $platform = $ARGV[1];
-- 
2.35.0.windows.1



^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-26 01:29  Andres Freund <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 1 reply; 37+ messages in thread

From: Andres Freund @ 2022-08-26 01:29 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Niyas Sait <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Hi,

On 2022-05-10 10:01:55 +0900, Michael Paquier wrote:
> On Mon, May 09, 2022 at 12:44:22PM +0100, Niyas Sait wrote:
> > Microsoft updated documentation [1] and clarified that ASLR cannot be
> > disabled for Arm64 targets.
> > 
> > Because ASLR can't be disabled on ARM, ARM64, or ARM64EC architectures,
> > > /DYNAMICBASE:NO isn't supported for these targets.
> 
> Better than nothing, I guess, though this does not stand as a reason
> explaining why this choice has been made.  And it looks like we will
> never know.  We are going to need more advanced testing to check that
> DYNAMICBASE is stable enough if we begin to enable it.  Perhaps we
> should really look at that for all the build targets we currently
> support rather than just ARM, for the most modern Win32 environments
> if we are going to cut support for most of the oldest releases..

I accidentally did a lot of testing with DYNAMICBASE - I accidentally
mistranslated MSBuildProject.pm's <RandomizedBaseAddress>false</> to adding
/DYNAMICBASE in the meson port. Survived several hundred CI cycles and dozens
of local runs without observing any problems related to that.

Which doesn't surprise me, given the way we reserve space in the new process.

Greetings,

Andres Freund





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-26 02:09  Michael Paquier <[email protected]>
  parent: Andres Freund <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Michael Paquier @ 2022-08-26 02:09 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Niyas Sait <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Thu, Aug 25, 2022 at 06:29:07PM -0700, Andres Freund wrote:
> I accidentally did a lot of testing with DYNAMICBASE - I accidentally
> mistranslated MSBuildProject.pm's <RandomizedBaseAddress>false</> to adding
> /DYNAMICBASE in the meson port. Survived several hundred CI cycles and dozens
> of local runs without observing any problems related to that.
> 
> Which doesn't surprise me, given the way we reserve space in the new process.

Still, we had problems with that in the past and I don't recall huge
changes in the way we allocate shmem on WIN32.  Could there be some
stuff in Windows itself that would explain more stability?  I would
not mind switching back to DYNAMICBASE on HEAD seeing your results.
That would simplify this thread's patch a bit as well.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-26 03:29  Andres Freund <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Andres Freund @ 2022-08-26 03:29 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Niyas Sait <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Hi,

On 2022-08-26 11:09:41 +0900, Michael Paquier wrote:
> On Thu, Aug 25, 2022 at 06:29:07PM -0700, Andres Freund wrote:
> > I accidentally did a lot of testing with DYNAMICBASE - I accidentally
> > mistranslated MSBuildProject.pm's <RandomizedBaseAddress>false</> to adding
> > /DYNAMICBASE in the meson port. Survived several hundred CI cycles and dozens
> > of local runs without observing any problems related to that.
> > 
> > Which doesn't surprise me, given the way we reserve space in the new process.
> 
> Still, we had problems with that in the past and I don't recall huge
> changes in the way we allocate shmem on WIN32.

It's really not great that 7f3e17b4827 disabled randomization without even a
comment as to why...

There actually seems to have been a lot of work in that area. See 617dc6d299c
/ bcbf2346d69 and as well as the retrying in 45e004fb4e39. Those weren't
prevented by us disabling ASLR.

Greetings,

Andres Freund





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-27 02:33  Michael Paquier <[email protected]>
  parent: Andres Freund <[email protected]>
  0 siblings, 2 replies; 37+ messages in thread

From: Michael Paquier @ 2022-08-27 02:33 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Niyas Sait <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Thu, Aug 25, 2022 at 08:29:43PM -0700, Andres Freund wrote:
> It's really not great that 7f3e17b4827 disabled randomization without even a
> comment as to why...

This story is on this thread, with some processes not able to start:
https://www.postgresql.org/message-id/BD0D89EC2438455C9DE0DC94D36912F4@maumau

> There actually seems to have been a lot of work in that area. See 617dc6d299c
> / bcbf2346d69 and as well as the retrying in 45e004fb4e39. Those weren't
> prevented by us disabling ASLR.

Indeed.  45e004f looks like the most interesting bit here.  FWIW, I
would not mind re-enabling that on HEAD, as of something like the
attached.  I have done a dozen of runs without seeing a test failure,
and knowing that we don't support anything older than Win10 makes me
feel safer about this change.  Any objections?
--
Michael


Attachments:

  [text/x-diff] 0001-Re-enable-ASLR-on-Windows.patch (973B, ../../YwmCj%[email protected]/2-0001-Re-enable-ASLR-on-Windows.patch)
  download | inline diff:
From 0fde47c2d4e65bd9f1f5a7b4607f8b8b95162242 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Sat, 27 Aug 2022 10:20:53 +0900
Subject: [PATCH] Re-enable ASLR on Windows

---
 src/tools/msvc/MSBuildProject.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
index 62acdda3a1..594729ceb7 100644
--- a/src/tools/msvc/MSBuildProject.pm
+++ b/src/tools/msvc/MSBuildProject.pm
@@ -347,7 +347,6 @@ sub WriteItemDefinitionGroup
       <ProgramDatabaseFile>.\\$cfgname\\$self->{name}\\$self->{name}.pdb</ProgramDatabaseFile>
       <GenerateMapFile>false</GenerateMapFile>
       <MapFileName>.\\$cfgname\\$self->{name}\\$self->{name}.map</MapFileName>
-      <RandomizedBaseAddress>false</RandomizedBaseAddress>
       <!-- Permit links to MinGW-built, 32-bit DLLs (default before VS2012). -->
       <ImageHasSafeExceptionHandlers/>
       <SubSystem>Console</SubSystem>
-- 
2.37.2



  [application/pgp-signature] signature.asc (833B, ../../YwmCj%[email protected]/3-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-27 03:02  Tom Lane <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 0 replies; 37+ messages in thread

From: Tom Lane @ 2022-08-27 03:02 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Michael Paquier <[email protected]> writes:
> Indeed.  45e004f looks like the most interesting bit here.  FWIW, I
> would not mind re-enabling that on HEAD, as of something like the
> attached.  I have done a dozen of runs without seeing a test failure,
> and knowing that we don't support anything older than Win10 makes me
> feel safer about this change.  Any objections?

We're early enough in the v16 cycle to have plenty of time to detect
any problems, so I see little reason not to try it.

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-27 19:27  Andres Freund <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 1 reply; 37+ messages in thread

From: Andres Freund @ 2022-08-27 19:27 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Niyas Sait <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Hi,

On 2022-08-27 11:33:51 +0900, Michael Paquier wrote:
> FWIW, I would not mind re-enabling that on HEAD, as of something like the
> attached.  I have done a dozen of runs without seeing a test failure, and
> knowing that we don't support anything older than Win10 makes me feel safer
> about this change.  Any objections?

+1. We don't have a choice about it on other architectures and it seems like a
bad idea to disable on its own.

I checked, and it looks like we didn't add --disable-dynamicbase, so ASLR
wasn't disabled for mingw builds.

Greetings,

Andres Freund





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-28 07:23  Michael Paquier <[email protected]>
  parent: Andres Freund <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Michael Paquier @ 2022-08-28 07:23 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Niyas Sait <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Sat, Aug 27, 2022 at 12:27:57PM -0700, Andres Freund wrote:
> I checked, and it looks like we didn't add --disable-dynamicbase, so ASLR
> wasn't disabled for mingw builds.

True enough.  I have applied the patch to re-enable that on HEAD.
Let's now see what happens in the next couple of days.  Popcorn is
ready here.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../YwsX3Jf89ucT%[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-28 14:09  Tom Lane <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Tom Lane @ 2022-08-28 14:09 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Michael Paquier <[email protected]> writes:
> True enough.  I have applied the patch to re-enable that on HEAD.
> Let's now see what happens in the next couple of days.  Popcorn is
> ready here.

Hmm:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-28%2010%3A30%3A29

Maybe that's just unrelated noise, but it sure looks weird: after
passing the core regression tests in "make check", it failed in
pg_upgrade's run with

diff -w -U3 H:/prog/bf/root/HEAD/pgsql.build/src/test/regress/expected/xml.out H:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/results/xml.out
--- H:/prog/bf/root/HEAD/pgsql.build/src/test/regress/expected/xml.out	Wed May 18 18:30:12 2022
+++ H:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/results/xml.out	Sun Aug 28 06:55:14 2022
@@ -1557,7 +1557,7 @@
 \\x
 SELECT * FROM XMLTABLE('*' PASSING '<e>pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post</e>' COLUMNS x xml PATH 'node()', y xml PATH '/');
 -[ RECORD 1 ]-----------------------------------------------------------
-x | pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post
+x | pre<?pi arg?><![CDATA[&ent1]]><!--c1--><n2>&amp;deep</n2>post
 y | <e>pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post</e>+
   | 
 

I don't recall ever seeing a failure quite like this.

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-28 15:41  Andres Freund <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Andres Freund @ 2022-08-28 15:41 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Hi,

On 2022-08-28 10:09:53 -0400, Tom Lane wrote:
> Michael Paquier <[email protected]> writes:
> > True enough.  I have applied the patch to re-enable that on HEAD.
> > Let's now see what happens in the next couple of days.  Popcorn is
> > ready here.
>
> Hmm:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-28%2010%3A30%3A29
>
> Maybe that's just unrelated noise, but it sure looks weird: after
> passing the core regression tests in "make check", it failed in
> pg_upgrade's run with
>
> diff -w -U3 H:/prog/bf/root/HEAD/pgsql.build/src/test/regress/expected/xml.out H:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/results/xml.out
> --- H:/prog/bf/root/HEAD/pgsql.build/src/test/regress/expected/xml.out	Wed May 18 18:30:12 2022
> +++ H:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/results/xml.out	Sun Aug 28 06:55:14 2022
> @@ -1557,7 +1557,7 @@
>  \\x
>  SELECT * FROM XMLTABLE('*' PASSING '<e>pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post</e>' COLUMNS x xml PATH 'node()', y xml PATH '/');
>  -[ RECORD 1 ]-----------------------------------------------------------
> -x | pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post
> +x | pre<?pi arg?><![CDATA[&ent1]]><!--c1--><n2>&amp;deep</n2>post
>  y | <e>pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post</e>+
>    |

Pretty weird, agreed. But hard to see how it could be caused by the
randomization change, except that perhaps it could highlight a preexisting
bug?

Greetings,

Andres Freund





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-28 15:51  Tom Lane <[email protected]>
  parent: Andres Freund <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Tom Lane @ 2022-08-28 15:51 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Andres Freund <[email protected]> writes:
> On 2022-08-28 10:09:53 -0400, Tom Lane wrote:
>> -x | pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post
>> +x | pre<?pi arg?><![CDATA[&ent1]]><!--c1--><n2>&amp;deep</n2>post

> Pretty weird, agreed. But hard to see how it could be caused by the
> randomization change, except that perhaps it could highlight a preexisting
> bug?

I have no idea either.  I agree there *shouldn't* be any connection,
so if ASLR is somehow triggering this then whatever is failing is
almost certainly buggy on its own terms.  But there's a lot of
moving parts here (mumble libxml mumble).  I'm going to wait to see
if it reproduces before spending much effort.

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-29 01:12  Michael Paquier <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Michael Paquier @ 2022-08-29 01:12 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Sun, Aug 28, 2022 at 11:51:19AM -0400, Tom Lane wrote:
> I have no idea either.  I agree there *shouldn't* be any connection,
> so if ASLR is somehow triggering this then whatever is failing is
> almost certainly buggy on its own terms.  But there's a lot of
> moving parts here (mumble libxml mumble).  I'm going to wait to see
> if it reproduces before spending much effort.

I have noticed that yesterday, but cannot think much about it.  This
basically changes the position of "<!--c1-->" for the first record,
leaving the second one untouched:
<!--c1--><?pi arg?><![CDATA[&ent1]]>
<?pi arg?><![CDATA[&ent1]]><!--c1-->

I am not used to xmltable(), but I wonder if there is something in one
of these support functions in xml.c that gets influenced by the
randomization.  That sounds a bit hairy as make check passed in
bowerbird, and I have noticed at least two other Windows hosts running
TAP that passed.  Or that's just something with libxml itself.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-29 23:48  Michael Paquier <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 2 replies; 37+ messages in thread

From: Michael Paquier @ 2022-08-29 23:48 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Mon, Aug 29, 2022 at 10:12:20AM +0900, Michael Paquier wrote:
> I have noticed that yesterday, but cannot think much about it.  This
> basically changes the position of "<!--c1-->" for the first record,
> leaving the second one untouched:
> <!--c1--><?pi arg?><![CDATA[&ent1]]>
> <?pi arg?><![CDATA[&ent1]]><!--c1-->
> 
> I am not used to xmltable(), but I wonder if there is something in one
> of these support functions in xml.c that gets influenced by the
> randomization.  That sounds a bit hairy as make check passed in
> bowerbird, and I have noticed at least two other Windows hosts running
> TAP that passed.  Or that's just something with libxml itself.

This is amazing.  The issue has showed up a second time in a row in
bowerbird, as of:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-29%2013%3A30%3A32

I don't know what to think about ASLR that manipulates the comment in
this XML object under VS 2017 (perhaps a compiler issue?), but it
should be possible to go back to green simply by removing "<!--c1-->"
from the input string.  Creating an extra output pattern here would be
very costly, as xml_2.out and xml.out have outputs for --with-libxml.

Would people object if I do that for now?
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-29 23:58  Tom Lane <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 1 reply; 37+ messages in thread

From: Tom Lane @ 2022-08-29 23:58 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Michael Paquier <[email protected]> writes:
> This is amazing.  The issue has showed up a second time in a row in
> bowerbird, as of:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-29%2013%3A30%3A32

That is fascinating.

> I don't know what to think about ASLR that manipulates the comment in
> this XML object under VS 2017 (perhaps a compiler issue?), but it
> should be possible to go back to green simply by removing "<!--c1-->"
> from the input string.  Creating an extra output pattern here would be
> very costly, as xml_2.out and xml.out have outputs for --with-libxml.

> Would people object if I do that for now?

Let's let it go for a few more runs.  I want to know whether it
reproduces 100% or only sometimes.

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-30 00:09  Andres Freund <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 0 replies; 37+ messages in thread

From: Andres Freund @ 2022-08-30 00:09 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; +Cc: Tom Lane <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Hi,

On 2022-08-30 08:48:38 +0900, Michael Paquier wrote:
> On Mon, Aug 29, 2022 at 10:12:20AM +0900, Michael Paquier wrote:
> > I have noticed that yesterday, but cannot think much about it.  This
> > basically changes the position of "<!--c1-->" for the first record,
> > leaving the second one untouched:
> > <!--c1--><?pi arg?><![CDATA[&ent1]]>
> > <?pi arg?><![CDATA[&ent1]]><!--c1-->
> > 
> > I am not used to xmltable(), but I wonder if there is something in one
> > of these support functions in xml.c that gets influenced by the
> > randomization.  That sounds a bit hairy as make check passed in
> > bowerbird, and I have noticed at least two other Windows hosts running
> > TAP that passed.  Or that's just something with libxml itself.
> 
> This is amazing.  The issue has showed up a second time in a row in
> bowerbird, as of:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-29%2013%3A30%3A32
> 
> I don't know what to think about ASLR that manipulates the comment in
> this XML object under VS 2017 (perhaps a compiler issue?), but it
> should be possible to go back to green simply by removing "<!--c1-->"
> from the input string.  Creating an extra output pattern here would be
> very costly, as xml_2.out and xml.out have outputs for --with-libxml.
> 
> Would people object if I do that for now?

I do. Clearly something is wrong, what do we gain by averting our eyes? An
alternative output file strikes me as an equally bad idea - it's not like this
is a case where there are legitimately different outputs?

Bowerbird is the only animal testing libxml with visual studio afaics. And CI
doesn't have libxml either.  So we have no idea if it's the msvc version that
matters.

Andrew, what version of libxml is installed? Afaict we don't see that anywhere
with msvc.

Greetings,

Andres Freund





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-30 00:33  Andres Freund <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Andres Freund @ 2022-08-30 00:33 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Hi,

On 2022-08-29 19:58:31 -0400, Tom Lane wrote:
> Michael Paquier <[email protected]> writes:
> > This is amazing.  The issue has showed up a second time in a row in
> > bowerbird, as of:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-29%2013%3A30%3A32
> 
> That is fascinating.

> > I don't know what to think about ASLR that manipulates the comment in
> > this XML object under VS 2017 (perhaps a compiler issue?), but it
> > should be possible to go back to green simply by removing "<!--c1-->"
> > from the input string.  Creating an extra output pattern here would be
> > very costly, as xml_2.out and xml.out have outputs for --with-libxml.
> 
> > Would people object if I do that for now?
> 
> Let's let it go for a few more runs.  I want to know whether it
> reproduces 100% or only sometimes.

The weirdest part is that it only happens as part of the the pg_upgrade test.

I just tested it in my test windows vm (win 10, vs 2019), with a build of
libxml I had around (2.9.7), and the regression tests passed both "normally"
and within pg_upgrade.

Greetings,

Andres Freund





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-30 01:35  Michael Paquier <[email protected]>
  parent: Andres Freund <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Michael Paquier @ 2022-08-30 01:35 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Mon, Aug 29, 2022 at 05:33:56PM -0700, Andres Freund wrote:
> The weirdest part is that it only happens as part of the the pg_upgrade test.

make check has just failed:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-30%2001%3A15%3A13

> I just tested it in my test windows vm (win 10, vs 2019), with a build of
> libxml I had around (2.9.7), and the regression tests passed both "normally"
> and within pg_upgrade.

bowerbird is feeling from c:\\prog\\3p64\\include\\libxml2 and
c:\\prog\\3p64\\lib\\libxml2.lib.  I am not sure which version of
libxml this is, and the other animals of MSVC don't use libxml so it
is not possible to correlate that only to VS 2017, either.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../Yw1pZ8X3rG%[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-30 02:00  Tom Lane <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Tom Lane @ 2022-08-30 02:00 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Michael Paquier <[email protected]> writes:
> On Mon, Aug 29, 2022 at 05:33:56PM -0700, Andres Freund wrote:
>> The weirdest part is that it only happens as part of the the pg_upgrade test.

> make check has just failed:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-30%2001%3A15%3A13

So it *is* probabilistic, which is pretty much what you'd expect
if ASLR triggers it.  That brings us no closer to understanding
what the mechanism is, though.

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-30 23:36  Michael Paquier <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 2 replies; 37+ messages in thread

From: Michael Paquier @ 2022-08-30 23:36 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Mon, Aug 29, 2022 at 10:00:10PM -0400, Tom Lane wrote:
> Michael Paquier <[email protected]> writes:
> > On Mon, Aug 29, 2022 at 05:33:56PM -0700, Andres Freund wrote:
> >> The weirdest part is that it only happens as part of the the pg_upgrade test.
> 
> > make check has just failed:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-30%2001%3A15%3A13
> 
> So it *is* probabilistic, which is pretty much what you'd expect
> if ASLR triggers it.  That brings us no closer to understanding
> what the mechanism is, though.

There have been more failures, always switching the input from
"pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post"
to "pre<?pi arg?><![CDATA[&ent1]]><!--c1--><n2>&amp;deep</n2>post".

Using a PATH of node() influences the output.  I am not verse unto
XMLTABLE, but could it be an issue where each node is parsed and we
have something like a qsort() applied on the pointer addresses for
each part or something like that, causing the output to become
unstable?
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-31 00:07  Tom Lane <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 1 reply; 37+ messages in thread

From: Tom Lane @ 2022-08-31 00:07 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Michael Paquier <[email protected]> writes:
> Using a PATH of node() influences the output.  I am not verse unto
> XMLTABLE, but could it be an issue where each node is parsed and we
> have something like a qsort() applied on the pointer addresses for
> each part or something like that, causing the output to become
> unstable?

I'm not sure.  I dug into this enough to find that the output from
this query is generated in xml.c lines 4635ff:

                    /* Concatenate serialized values */
                    initStringInfo(&str);
                    for (int i = 0; i < count; i++)
                    {
                        textstr =
                            xml_xmlnodetoxmltype(xpathobj->nodesetval->nodeTab[i],
                                                 xtCxt->xmlerrcxt);

                        appendStringInfoText(&str, textstr);
                    }
                    cstr = str.data;

So evidently, the problem occurs because the elements of the
nodesetval->nodeTab[] array are in a different order than we expect.

I looked into <xpath.h> and found the definition of the "nodesetval"
struct, and the comments are eye-opening to say the least:

/*
 * A node-set (an unordered collection of nodes without duplicates).
 */
typedef struct _xmlNodeSet xmlNodeSet;
typedef xmlNodeSet *xmlNodeSetPtr;
struct _xmlNodeSet {
    int nodeNr;			/* number of nodes in the set */
    int nodeMax;		/* size of the array as allocated */
    xmlNodePtr *nodeTab;	/* array of nodes in no particular order */
    /* @@ with_ns to check wether namespace nodes should be looked at @@ */
};

It seems like maybe we're relying on an ordering we should not.
Yet surely the ordering of the pieces of this output is meaningful?
Are we using the wrong libxml API to create this result?

Anyway, I'm now suspicious that we've accidentally exposed a logic
bug in the XMLTABLE code, rather than anything wrong with the
ASLR stuff.

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-31 00:15  Michael Paquier <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 1 reply; 37+ messages in thread

From: Michael Paquier @ 2022-08-31 00:15 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Wed, Aug 31, 2022 at 08:36:01AM +0900, Michael Paquier wrote:
> There have been more failures, always switching the input from
> "pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post"
> to "pre<?pi arg?><![CDATA[&ent1]]><!--c1--><n2>&amp;deep</n2>post".
> 
> Using a PATH of node() influences the output.  I am not verse unto
> XMLTABLE, but could it be an issue where each node is parsed and we
> have something like a qsort() applied on the pointer addresses for
> each part or something like that, causing the output to become
> unstable?

Hmm.  I think that I may have an idea here after looking at our xml.c
and xpath.c in libxml2/.  From what I understand, we process the PATH
through XmlTableGetValue() that builds a XML node path in
xmlXPathCompiledEval().  The interesting part comes from libxml2's
xmlXPathCompiledEvalInternal(), where I think we don't apply a sort on
the contents generated.  Hence, I am wondering if the solution here
would be to do one xmlXPathNodeSetSort(xpathobj->nodesetval) after
compiling the path with xmlXPathCompiledEval() in XmlTableGetValue().
This should ensure that the items are ordered even if ASLR mixes if
the pointer positions.

A complete solution would involve more code paths, but we'd need only
one change in XmlTableGetValue() for the current regression tests to
work.  I don't have an environment where I can reproduce that, so that
would be up to the buildfarm to stress this solution..

Thoughts?
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-31 00:16  Michael Paquier <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 0 replies; 37+ messages in thread

From: Michael Paquier @ 2022-08-31 00:16 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Tue, Aug 30, 2022 at 08:07:27PM -0400, Tom Lane wrote:
> It seems like maybe we're relying on an ordering we should not.
> Yet surely the ordering of the pieces of this output is meaningful?
> Are we using the wrong libxml API to create this result?
> 
> Anyway, I'm now suspicious that we've accidentally exposed a logic
> bug in the XMLTABLE code, rather than anything wrong with the
> ASLR stuff.

Funny.  I have reached basically the same conclusion as you a couple
of minutes ago, but I also think that I have found what we need to do
here to ensure the ordering of the nodes generated by the libxml
code.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-31 00:28  Tom Lane <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Tom Lane @ 2022-08-31 00:28 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Michael Paquier <[email protected]> writes:
> Hence, I am wondering if the solution here
> would be to do one xmlXPathNodeSetSort(xpathobj->nodesetval) after
> compiling the path with xmlXPathCompiledEval() in XmlTableGetValue().

Hmm, I see that function declared in <xpathInternals.h>, which
sure doesn't give me a warm feeling that we're supposed to call it
directly.  But maybe there's an approved way to get the result.

Or perhaps this test case is wrong, and instead of "node()" we
need to write something that specifies a sorted result?

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-31 06:09  Michael Paquier <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 37+ messages in thread

From: Michael Paquier @ 2022-08-31 06:09 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

On Tue, Aug 30, 2022 at 08:28:58PM -0400, Tom Lane wrote:
> Hmm, I see that function declared in <xpathInternals.h>, which
> sure doesn't give me a warm feeling that we're supposed to call it
> directly.  But maybe there's an approved way to get the result.
> 
> Or perhaps this test case is wrong, and instead of "node()" we
> need to write something that specifies a sorted result?

I am no specialist on this matter..  Still, looking around, this looks
to be the kind of job that xsl:sort would do?  We cannot use it here
for an XML-only context, though, and it does not seem like there is a
direct way to enforce the ordering per the nature of the XPath
language, either.  I may be missing something of course.

Please note that I am also getting a bit the cold feet with the idea
of enforcing a sorting of the elements in the generated output,
actually, as this could have a performance penalty for all users
particularly on large blobs of data.

At the end, I'd like to think that it would be wiser to just remove
entirely the test for node() or reduce the contents of the string to
be able to strictly control the output order (say a simple
'<e>pre<![CDATA[&ent1]]>post</e>' would do the trick).  I cannot think
about a better idea now.  Note that removing the test case where we
have node() has no impact on the coverage of xml.c.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* Re: [PATCH] Add native windows on arm64 support
@ 2022-08-31 17:33  Tom Lane <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 0 replies; 37+ messages in thread

From: Tom Lane @ 2022-08-31 17:33 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Niyas Sait <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>

Michael Paquier <[email protected]> writes:
> At the end, I'd like to think that it would be wiser to just remove
> entirely the test for node() or reduce the contents of the string to
> be able to strictly control the output order (say a simple
> '<e>pre<![CDATA[&ent1]]>post</e>' would do the trick).  I cannot think
> about a better idea now.  Note that removing the test case where we
> have node() has no impact on the coverage of xml.c.

Yeah, I confirm that: local code-coverage testing shows no change
in the number of lines reached in xml.c when I remove that column:

-SELECT * FROM XMLTABLE('*' PASSING '<e>pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post</e>' COLUMNS x xml PATH 'node()', y xml PATH '/');
+SELECT * FROM XMLTABLE('*' PASSING '<e>pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post</e>' COLUMNS y xml PATH '/');

Dropping the query altogether does result in a reduction in the
number of lines hit.

I did not check the other XMLTABLE infrastructure, so what probably
is best to do is keep the two output columns but change 'node()'
to something with a more stable result; any preferences?

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 37+ messages in thread

* [PATCH v4 05/12] Improve sentences in overview of system configuration parameters
@ 2023-09-25 20:32  Karl O. Pinc <[email protected]>
  0 siblings, 0 replies; 37+ messages in thread

From: Karl O. Pinc @ 2023-09-25 20:32 UTC (permalink / raw)

Get rid of "we" wording.  Remove extra words in sentences.

Line break after end of each sentence to ease future patch reading.
---
 doc/src/sgml/config.sgml | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6bc1b215db..97f9838bfb 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -10,9 +10,10 @@
 
   <para>
    There are many configuration parameters that affect the behavior of
-   the database system. In the first section of this chapter we
-   describe how to interact with configuration parameters. The subsequent sections
-   discuss each parameter in detail.
+   the database system.
+   The first section of this chapter describes how to interact with
+   configuration parameters.
+   Subsequent sections discuss each parameter in detail.
   </para>
 
   <sect1 id="config-setting">
-- 
2.30.2


--MP_/OOXZvOwbpccKfGOtE9/SwX6
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=v4-0006-Provide-examples-of-listing-all-settings.patch



^ permalink  raw  reply  [nested|flat] 37+ messages in thread


end of thread, other threads:[~2023-09-25 20:32 UTC | newest]

Thread overview: 37+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-10-16 04:04 [PATCH 3/5] Remove entries that haven't been used for a certain time Kyotaro Horiguchi <[email protected]>
2022-04-07 17:42 Re: [PATCH] Add native windows on arm64 support Tom Lane <[email protected]>
2022-04-07 17:53 ` Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2022-04-19 14:22   ` Re: [PATCH] Add native windows on arm64 support Niyas Sait <[email protected]>
2022-04-20 00:28     ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-04-20 09:43       ` Re: [PATCH] Add native windows on arm64 support Niyas Sait <[email protected]>
2022-04-21 04:07         ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-04-21 09:21           ` Re: [PATCH] Add native windows on arm64 support Niyas Sait <[email protected]>
2022-04-25 01:17             ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-05-09 11:44               ` Re: [PATCH] Add native windows on arm64 support Niyas Sait <[email protected]>
2022-05-10 01:01                 ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-07-14 08:36                   ` Re: [PATCH] Add native windows on arm64 support Niyas Sait <[email protected]>
2022-08-26 01:29                   ` Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2022-08-26 02:09                     ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-08-26 03:29                       ` Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2022-08-27 02:33                         ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-08-27 03:02                           ` Re: [PATCH] Add native windows on arm64 support Tom Lane <[email protected]>
2022-08-27 19:27                           ` Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2022-08-28 07:23                             ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-08-28 14:09                               ` Re: [PATCH] Add native windows on arm64 support Tom Lane <[email protected]>
2022-08-28 15:41                                 ` Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2022-08-28 15:51                                   ` Re: [PATCH] Add native windows on arm64 support Tom Lane <[email protected]>
2022-08-29 01:12                                     ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-08-29 23:48                                       ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-08-29 23:58                                         ` Re: [PATCH] Add native windows on arm64 support Tom Lane <[email protected]>
2022-08-30 00:33                                           ` Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2022-08-30 01:35                                             ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-08-30 02:00                                               ` Re: [PATCH] Add native windows on arm64 support Tom Lane <[email protected]>
2022-08-30 23:36                                                 ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-08-31 00:07                                                   ` Re: [PATCH] Add native windows on arm64 support Tom Lane <[email protected]>
2022-08-31 00:16                                                     ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-08-31 00:15                                                   ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-08-31 00:28                                                     ` Re: [PATCH] Add native windows on arm64 support Tom Lane <[email protected]>
2022-08-31 06:09                                                       ` Re: [PATCH] Add native windows on arm64 support Michael Paquier <[email protected]>
2022-08-31 17:33                                                         ` Re: [PATCH] Add native windows on arm64 support Tom Lane <[email protected]>
2022-08-30 00:09                                         ` Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2023-09-25 20:32 [PATCH v4 05/12] Improve sentences in overview of system configuration parameters Karl O. Pinc <[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