Re: making relfilenodes 56 bits

Thomas Munro <thomas.munro@gmail.com>

From: Thomas Munro <thomas.munro@gmail.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Andres Freund <andres@anarazel.de>, Heikki Linnakangas <hlinnaka@iki.fi>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2022-09-28T03:52:56Z
Lists: pgsql-hackers
Hi Dilip,

I am very happy to see these commits.  Here's some belated review for
the tombstone-removal patch.

> v7-0004-Don-t-delay-removing-Tombstone-file-until-next.patch

More things you can remove:

 * sync_unlinkfiletag in struct SyncOps
 * the macro UNLINKS_PER_ABSORB
 * global variable pendingUnlinks

This comment after the question mark is obsolete:

                * XXX should we CHECK_FOR_INTERRUPTS in this loop?
Escaping with an
                * error in the case of SYNC_UNLINK_REQUEST would leave the
                * no-longer-used file still present on disk, which
would be bad, so
                * I'm inclined to assume that the checkpointer will
always empty the
                * queue soon.

(I think if the answer to the question is now yes, then we should
replace the stupid sleep with a condition variable sleep, but there's
another thread about that somewhere).

In a couple of places in dbcommands.c you could now make this change:

        /*
-        * Force a checkpoint before starting the copy. This will
force all dirty
-        * buffers, including those of unlogged tables, out to disk, to ensure
-        * source database is up-to-date on disk for the copy.
-        * FlushDatabaseBuffers() would suffice for that, but we also want to
-        * process any pending unlink requests. Otherwise, if a checkpoint
-        * happened while we're copying files, a file might be deleted just when
-        * we're about to copy it, causing the lstat() call in copydir() to fail
-        * with ENOENT.
+        * Force all dirty buffers, including those of unlogged tables, out to
+        * disk, to ensure source database is up-to-date on disk for the copy.
         */
-       RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_FORCE |
-                                         CHECKPOINT_WAIT |
CHECKPOINT_FLUSH_ALL);
+       FlushDatabaseBuffers(src_dboid);

More obsolete comments you could change:

         * If we were copying database at block levels then drop pages for the
         * destination database that are in the shared buffer cache.  And tell
-->      * checkpointer to forget any pending fsync and unlink
requests for files

-->     * Tell checkpointer to forget any pending fsync and unlink requests for
        * files in the database; else the fsyncs will fail at next
checkpoint, or
        * worse, it will delete file

In tablespace.c I think you could now make this change:

        if (!destroy_tablespace_directories(tablespaceoid, false))
        {
-               /*
-                * Not all files deleted?  However, there can be
lingering empty files
-                * in the directories, left behind by for example DROP
TABLE, that
-                * have been scheduled for deletion at next checkpoint
(see comments
-                * in mdunlink() for details).  We could just delete
them immediately,
-                * but we can't tell them apart from important data
files that we
-                * mustn't delete.  So instead, we force a checkpoint
which will clean
-                * out any lingering files, and try again.
-                */
-               RequestCheckpoint(CHECKPOINT_IMMEDIATE |
CHECKPOINT_FORCE | CHECKPOINT_WAIT);
-
+#ifdef WIN32
                /*
                 * On Windows, an unlinked file persists in the
directory listing
                 * until no process retains an open handle for the
file.  The DDL
@@ -523,6 +513,7 @@ DropTableSpace(DropTableSpaceStmt *stmt)

                /* And now try again. */
                if (!destroy_tablespace_directories(tablespaceoid, false))
+#endif
                {
                        /* Still not empty, the files must be important then */
                        ereport(ERROR,



Commits

  1. Fix typos in comments, code and documentation

  2. Increase width of RelFileNumbers from 32 bits to 56 bits.

  3. Move RelFileNumber declarations to common/relpath.h.

  4. Include RelFileLocator fields individually in BufferTag.

  5. Convert macros to static inline functions (buf_internals.h)

  6. Remove the restriction that the relmap must be 512 bytes.

  7. Change internal RelFileNode references to RelFileNumber or RelFileLocator.

  8. Reorder subskiplsn in pg_subscription to avoid alignment issues.

  9. Fix DROP {DATABASE,TABLESPACE} on Windows.

  10. Restructure smgr API as per recent proposal. smgr no longer depends on