Re: making relfilenodes 56 bits

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,