Re: Do XID sequences need to be contiguous?

2019-11-29 Thread Amit Kapila
On Fri, Nov 29, 2019 at 12:22 AM Mark Dilger  wrote:
>
> Hackers,
>
> While working on the problem of XID wraparound within the LISTEN/NOTIFY
> system, I tried to increment XIDs by more than one per transaction.
> This leads to a number of test failures, many which look like:
>
> +ERROR:  could not access status of transaction 7485
> +DETAIL:  Could not read from file "pg_subtrans/" at offset 24576:
> read too few bytes.
>
> I might not have read the right documentation, but
>
> I do not see anything in src/backend/access/transam/README nor elsewhere
> documenting a design decision or assumption that transaction IDs must
> be assigned contiguously.  I suppose this is such a fundamental
> assumption that it is completely implicit and nobody thought to document
> it, but I'd like to check for two reasons:
>
> First, I'd like a good method of burning through transaction ids in
> tests designed to check for problems in XID wrap-around.
>

As Tom pointed out and as mentioned in the comments "If we are
allocating the first XID of a new page of the commit log, zero out
that commit-log page before returning.", we need to take care of
extending the CLOG while advancing TransactionIds.   I have some old
script for burning transactionid's which I am attaching here.  It
might help you.  I think this is provided long back by Jeff Janes.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


transactionid_burner_v1.patch
Description: Binary data


Re: Do XID sequences need to be contiguous?

2019-11-28 Thread Tom Lane
Mark Dilger  writes:
> While working on the problem of XID wraparound within the LISTEN/NOTIFY
> system, I tried to increment XIDs by more than one per transaction. 
> This leads to a number of test failures, many which look like:

IIRC, the XID-creation logic is designed to initialize the next clog
page whenever it allocates an exact-multiple-of-BLCKSZ*4 transaction
number.  Skipping over such numbers would create trouble.

> First, I'd like a good method of burning through transaction ids in
> tests designed to check for problems in XID wrap-around.

Don't "burn through them".  Stop the cluster and use pg_resetwal to
set the XID counter wherever you want it.  (You might need to set it
just before a page or segment boundary; I'm not sure if pg_resetwal
has any logic of its own to initialize a new CLOG page/file when you
move the counter this way.  Perhaps it's worth improving that.)

> Second, I'd like to add Asserts where appropriate regarding this
> assumption.

I'm not excited about that, and it's *certainly* not a problem that
justifies additional configure infrastructure.

regards, tom lane




Do XID sequences need to be contiguous?

2019-11-28 Thread Mark Dilger

Hackers,

While working on the problem of XID wraparound within the LISTEN/NOTIFY
system, I tried to increment XIDs by more than one per transaction. 
This leads to a number of test failures, many which look like:


+ERROR:  could not access status of transaction 7485
+DETAIL:  Could not read from file "pg_subtrans/" at offset 24576: 
read too few bytes.


I might not have read the right documentation, but

I do not see anything in src/backend/access/transam/README nor elsewhere
documenting a design decision or assumption that transaction IDs must
be assigned contiguously.  I suppose this is such a fundamental
assumption that it is completely implicit and nobody thought to document
it, but I'd like to check for two reasons:

First, I'd like a good method of burning through transaction ids in
tests designed to check for problems in XID wrap-around.

Second, I'd like to add Asserts where appropriate regarding this
assumption.  It seems strange to me that I should have gotten as far
as a failing read() without having tripped an Assert somewhere along the
way.

To duplicate the errors I hit, you can either apply this simple change:


diff --git a/src/include/access/transam.h b/src/include/access/transam.h
index 33fd052156..360b7335bb 100644
--- a/src/include/access/transam.h
+++ b/src/include/access/transam.h
@@ -83,7 +83,7 @@ FullTransactionIdFromEpochAndXid(uint32 epoch, 
TransactionId xid)

 static inline void
 FullTransactionIdAdvance(FullTransactionId *dest)
 {
-   dest->value++;
+   dest->value += 2;
while (XidFromFullTransactionId(*dest) < FirstNormalTransactionId)
dest->value++;
 }


or apply the much larger WIP patch, attached, and then be sure to
provide the --enable-xidcheck flag to configure before building.

--
Mark Dilger
diff --git a/configure b/configure
index 1d88983b34..909907127a 100755
--- a/configure
+++ b/configure
@@ -841,6 +841,7 @@ with_CC
 with_llvm
 enable_depend
 enable_cassert
+enable_xidcheck
 enable_thread_safety
 with_icu
 with_tcl
@@ -1521,6 +1522,7 @@ Optional Features:
   --enable-tap-tests  enable TAP tests (requires Perl and IPC::Run)
   --enable-depend turn on automatic dependency tracking
   --enable-cassertenable assertion checks (for debugging)
+  --enable-xidcheck   enable transactionid checks (for debugging)
   --disable-thread-safety disable thread-safety in client libraries
   --disable-largefile omit support for large files
 
@@ -7197,6 +7199,36 @@ fi
 
 
 
+#
+# Enable transactionid checks
+#
+
+
+# Check whether --enable-xidcheck was given.
+if test "${enable_xidcheck+set}" = set; then :
+  enableval=$enable_xidcheck;
+  case $enableval in
+yes)
+
+$as_echo "#define USE_XID_CHECKING 1" >>confdefs.h
+
+  ;;
+no)
+  :
+  ;;
+*)
+  as_fn_error $? "no argument expected for --enable-xidcheck option" 
"$LINENO" 5
+  ;;
+  esac
+
+else
+  enable_xidcheck=no
+
+fi
+
+
+
+
 #
 # Include directories
 #
diff --git a/configure.in b/configure.in
index a2cb20b5e3..5aac6c24f1 100644
--- a/configure.in
+++ b/configure.in
@@ -680,6 +680,14 @@ PGAC_ARG_BOOL(enable, cassert, no, [enable assertion 
checks (for debugging)],
  [Define to 1 to build with assertion checks. 
(--enable-cassert)])])
 
 
+#
+# Enable transactionid checks
+#
+PGAC_ARG_BOOL(enable, xidcheck, no, [enable transactionid checks (for 
debugging)],
+  [AC_DEFINE([USE_XID_CHECKING], 1,
+ [Define to 1 to build with transactionid checks. 
(--enable-xidcheck)])])
+
+
 #
 # Include directories
 #
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index d4d1fe45cc..a3dbe27640 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9246,6 +9246,26 @@ dynamic_library_path = 
'C:\tools\postgresql;H:\my_project\lib;$libdir'
   
  
 
+ 
+  debug_xidcheck (boolean)
+  
+   debug_xidcheck configuration 
parameter
+  
+  
+  
+   
+Reports whether PostgreSQL has been built
+with transaction id checking enabled. That is the case if the
+macro USE_XID_CHECKING is defined
+when PostgreSQL is built (accomplished
+e.g. by the configure option
+--enable-xidcheck). By
+default PostgreSQL is built without
+transaction id checking.
+   
+  
+ 
+
  
   integer_datetimes (boolean)
   
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index 9c10a897f1..eedfd021a0 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -1524,6 +1524,19 @@ build-postgresql:

   
 
+  
+   --enable-xidcheck
+   
+
+ Enables transaction ID checks in the server, 
which test
+ the 64-bit transaction ID management.  This may prove valuable for
+ code development purposes, but enabling this feature will consume a 
small
+ amount of sh