add-transient-smgrrelations.c

text/x-patch

Filename: add-transient-smgrrelations.c
Type: text/x-patch
Part: 1
Message: Re: smgrsettransient mechanism is full of bugs

Patch

Same data as JSON: GET /api/v1/attachments/:id/patch the parsed metadata as JSON — format, series position, per-file stats; never the diff bytes. API reference →
Format: unified
File+
src/backend/access/transam/xact.c 0 0
src/backend/postmaster/bgwriter.c 0 0
src/backend/postmaster/checkpointer.c 0 0
src/backend/postmaster/walwriter.c 0 0
src/backend/storage/smgr/smgr.c 0 0
src/include/storage/smgr.h 0 0
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index e7a6606ec3b33d285ac032c18fb64e557abdf821..c24df3f38c2bbf2e0becfe0394cb92fd6b6566d1 100644
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*************** CommitTransaction(void)
*** 1949,1955 ****
  	AtEOXact_SPI(true);
  	AtEOXact_on_commit_actions(true);
  	AtEOXact_Namespace(true);
! 	/* smgrcommit already done */
  	AtEOXact_Files();
  	AtEOXact_ComboCid();
  	AtEOXact_HashTables(true);
--- 1949,1955 ----
  	AtEOXact_SPI(true);
  	AtEOXact_on_commit_actions(true);
  	AtEOXact_Namespace(true);
! 	AtEOXact_SMgr();
  	AtEOXact_Files();
  	AtEOXact_ComboCid();
  	AtEOXact_HashTables(true);
*************** PrepareTransaction(void)
*** 2202,2208 ****
  	AtEOXact_SPI(true);
  	AtEOXact_on_commit_actions(true);
  	AtEOXact_Namespace(true);
! 	/* smgrcommit already done */
  	AtEOXact_Files();
  	AtEOXact_ComboCid();
  	AtEOXact_HashTables(true);
--- 2202,2208 ----
  	AtEOXact_SPI(true);
  	AtEOXact_on_commit_actions(true);
  	AtEOXact_Namespace(true);
! 	AtEOXact_SMgr();
  	AtEOXact_Files();
  	AtEOXact_ComboCid();
  	AtEOXact_HashTables(true);
*************** AbortTransaction(void)
*** 2348,2353 ****
--- 2348,2354 ----
  		AtEOXact_SPI(false);
  		AtEOXact_on_commit_actions(false);
  		AtEOXact_Namespace(false);
+ 		AtEOXact_SMgr();
  		AtEOXact_Files();
  		AtEOXact_ComboCid();
  		AtEOXact_HashTables(false);
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 748fd85edb0d7175e5ffd99288b92eddd8706d20..709ccf1f2561907f96f1ae2bf8ddc47e326efbb1 100644
*** a/src/backend/postmaster/bgwriter.c
--- b/src/backend/postmaster/bgwriter.c
*************** BackgroundWriterMain(void)
*** 183,188 ****
--- 183,189 ----
  							 false, true);
  		/* we needn't bother with the other ResourceOwnerRelease phases */
  		AtEOXact_Buffers(false);
+ 		AtEOXact_SMgr();
  		AtEOXact_Files();
  		AtEOXact_HashTables(false);
  
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index c5f32059a792bc407bd01e1fab26c180148ebf3d..18e6a4e8c4be1c1e6041cd854a55f367ffd4ba6e 100644
*** a/src/backend/postmaster/checkpointer.c
--- b/src/backend/postmaster/checkpointer.c
*************** CheckpointerMain(void)
*** 291,296 ****
--- 291,297 ----
  							 false, true);
  		/* we needn't bother with the other ResourceOwnerRelease phases */
  		AtEOXact_Buffers(false);
+ 		AtEOXact_SMgr();
  		AtEOXact_Files();
  		AtEOXact_HashTables(false);
  
diff --git a/src/backend/postmaster/walwriter.c b/src/backend/postmaster/walwriter.c
index 43139017c276648ec292a81a4684e605701e2205..c3e15ef7595de7373e319540691304285672c178 100644
*** a/src/backend/postmaster/walwriter.c
--- b/src/backend/postmaster/walwriter.c
*************** WalWriterMain(void)
*** 188,193 ****
--- 188,194 ----
  							 false, true);
  		/* we needn't bother with the other ResourceOwnerRelease phases */
  		AtEOXact_Buffers(false);
+ 		AtEOXact_SMgr();
  		AtEOXact_Files();
  		AtEOXact_HashTables(false);
  
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 6319d1e8589138be32e498e77cc4ce484211c1a1..5dff8b3702ee64c355a050aa76242b99b6eea151 100644
*** a/src/backend/storage/smgr/smgr.c
--- b/src/backend/storage/smgr/smgr.c
*************** static const int NSmgr = lengthof(smgrsw
*** 76,86 ****
--- 76,90 ----
  
  /*
   * Each backend has a hashtable that stores all extant SMgrRelation objects.
+  * In addition, "unowned" SMgrRelation objects are chained together in a list.
   */
  static HTAB *SMgrRelationHash = NULL;
  
+ static SMgrRelation first_unowned_reln = NULL;
+ 
  /* local function prototypes */
  static void smgrshutdown(int code, Datum arg);
+ static void remove_from_unowned_list(SMgrRelation reln);
  
  
  /*
*************** smgrshutdown(int code, Datum arg)
*** 124,130 ****
  /*
   *	smgropen() -- Return an SMgrRelation object, creating it if need be.
   *
!  *		This does not attempt to actually open the object.
   */
  SMgrRelation
  smgropen(RelFileNode rnode, BackendId backend)
--- 128,134 ----
  /*
   *	smgropen() -- Return an SMgrRelation object, creating it if need be.
   *
!  *		This does not attempt to actually open the underlying file.
   */
  SMgrRelation
  smgropen(RelFileNode rnode, BackendId backend)
*************** smgropen(RelFileNode rnode, BackendId ba
*** 144,149 ****
--- 148,154 ----
  		ctl.hash = tag_hash;
  		SMgrRelationHash = hash_create("smgr relation table", 400,
  									   &ctl, HASH_ELEM | HASH_FUNCTION);
+ 		first_unowned_reln = NULL;
  	}
  
  	/* Look up or create an entry */
*************** smgropen(RelFileNode rnode, BackendId ba
*** 168,173 ****
--- 173,182 ----
  		/* mark it not open */
  		for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
  			reln->md_fd[forknum] = NULL;
+ 
+ 		/* place it at head of unowned list (to make smgrsetowner cheap) */
+ 		reln->next_unowned_reln = first_unowned_reln;
+ 		first_unowned_reln = reln;
  	}
  
  	return reln;
*************** smgropen(RelFileNode rnode, BackendId ba
*** 182,195 ****
--- 191,212 ----
  void
  smgrsetowner(SMgrRelation *owner, SMgrRelation reln)
  {
+ 	/* We don't currently support "disowning" an SMgrRelation here */
+ 	Assert(owner != NULL);
+ 
  	/*
  	 * First, unhook any old owner.  (Normally there shouldn't be any, but it
  	 * seems possible that this can happen during swap_relation_files()
  	 * depending on the order of processing.  It's ok to close the old
  	 * relcache entry early in that case.)
+ 	 *
+ 	 * If there isn't an old owner, then the reln should be in the unowned
+ 	 * list, and we need to remove it.
  	 */
  	if (reln->smgr_owner)
  		*(reln->smgr_owner) = NULL;
+ 	else
+ 		remove_from_unowned_list(reln);
  
  	/* Now establish the ownership relationship. */
  	reln->smgr_owner = owner;
*************** smgrsetowner(SMgrRelation *owner, SMgrRe
*** 197,202 ****
--- 214,251 ----
  }
  
  /*
+  * remove_from_unowned_list -- unlink an SMgrRelation from the unowned list
+  *
+  * If the reln is not present in the list, nothing happens.  Typically this
+  * would be caller error, but there seems no reason to throw an error.
+  *
+  * In the worst case this could be rather slow; but in all the cases that seem
+  * likely to be performance-critical, the reln being sought will actually be
+  * first in the list.  Furthermore, the number of unowned relns touched in any
+  * one transaction shouldn't be all that high typically.  So it doesn't seem
+  * worth expending the additional space and management logic needed for a
+  * doubly-linked list.
+  */
+ static void
+ remove_from_unowned_list(SMgrRelation reln)
+ {
+ 	SMgrRelation *link;
+ 	SMgrRelation cur;
+ 
+ 	for (link = &first_unowned_reln, cur = *link;
+ 		 cur != NULL;
+ 		 link = &cur->next_unowned_reln, cur = *link)
+ 	{
+ 		if (cur == reln)
+ 		{
+ 			*link = cur->next_unowned_reln;
+ 			cur->next_unowned_reln = NULL;
+ 			break;
+ 		}
+ 	}
+ }
+ 
+ /*
   *	smgrexists() -- Does the underlying file for a fork exist?
   */
  bool
*************** smgrclose(SMgrRelation reln)
*** 219,224 ****
--- 268,276 ----
  
  	owner = reln->smgr_owner;
  
+ 	if (!owner)
+ 		remove_from_unowned_list(reln);
+ 
  	if (hash_search(SMgrRelationHash,
  					(void *) &(reln->smgr_rnode),
  					HASH_REMOVE, NULL) == NULL)
*************** smgrpostckpt(void)
*** 600,602 ****
--- 652,680 ----
  			(*(smgrsw[i].smgr_post_ckpt)) ();
  	}
  }
+ 
+ /*
+  * AtEOXact_SMgr
+  *
+  * This routine is called during transaction commit or abort (it doesn't
+  * particularly care which).  All transient SMgrRelation objects are closed.
+  *
+  * We do this as a compromise between wanting transient SMgrRelations to
+  * live awhile (to amortize the costs of blind writes of multiple blocks)
+  * and needing them to not live forever (since we're probably holding open
+  * a kernel file descriptor for the underlying file, and we need to ensure
+  * that gets closed reasonably soon if the file gets deleted).
+  */
+ void
+ AtEOXact_SMgr(void)
+ {
+ 	/*
+ 	 * Zap all unowned SMgrRelations.  We rely on smgrclose() to remove each
+ 	 * one from the list.
+ 	 */
+ 	while (first_unowned_reln != NULL)
+ 	{
+ 		Assert(first_unowned_reln->smgr_owner == NULL);
+ 		smgrclose(first_unowned_reln);
+ 	}
+ }
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 92b9198d5428c1f51617f595d062da1bc8af5106..9eccf7671ed99be4c068e3df50a75c21c1ce9e51 100644
*** a/src/include/storage/smgr.h
--- b/src/include/storage/smgr.h
***************
*** 33,38 ****
--- 33,41 ----
   * without having to make the smgr explicitly aware of relcache.  There
   * can't be more than one "owner" pointer per SMgrRelation, but that's
   * all we need.
+  *
+  * SMgrRelations that do not have an "owner" are considered to be transient,
+  * and are deleted at end of transaction.
   */
  typedef struct SMgrRelationData
  {
*************** typedef struct SMgrRelationData
*** 63,68 ****
--- 66,74 ----
  
  	/* for md.c; NULL for forks that are not open */
  	struct _MdfdVec *md_fd[MAX_FORKNUM + 1];
+ 
+ 	/* if unowned, list link in list of all unowned SMgrRelations */
+ 	struct SMgrRelationData *next_unowned_reln;
  } SMgrRelationData;
  
  typedef SMgrRelationData *SMgrRelation;
*************** extern void smgrimmedsync(SMgrRelation r
*** 95,100 ****
--- 101,107 ----
  extern void smgrpreckpt(void);
  extern void smgrsync(void);
  extern void smgrpostckpt(void);
+ extern void AtEOXact_SMgr(void);
  
  
  /* internals: move me elsewhere -- ay 7/94 */