Re: Extensible sync handler registration (register_sync_handler)

Hi,

I have been studying and reviewing your patch for the past few days
and I have one hardening suggestion from testing the v2 patch.

> diff --git a/src/include/storage/sync.h b/src/include/storage/sync.h
> @@ -55,6 +70,25 @@ typedef struct FileTag
> uint64 segno;
> } FileTag;

> +/*
> + * Dispatch table entry for a sync handler. Public so extensions can
> + * define their own SyncOps and pass them to register_sync_handler().
> + *
> + * sync_syncfiletag is required. sync_unlinkfiletag and
> + * sync_filetagmatches may be NULL if the handler does not support
> + * SYNC_UNLINK_REQUEST or SYNC_FILTER_REQUEST respectively, matching
> + * the pattern of the built-in CLOG/commit_ts/multixact handlers which
> + * only define sync_syncfiletag.
> + */
> +typedef struct SyncOps
> +{
> + int (*sync_syncfiletag) (const FileTag *ftag, char *path);
> + int (*sync_unlinkfiletag) (const FileTag *ftag, char *path);
> + bool (*sync_filetagmatches) (const FileTag *ftag,
> + const FileTag *candidate);
> +} SyncOps;
> +
> +extern void InitSyncHandlers(void);

Here the comment says "sync_unlinkfiletag and sync_filetagmatches may
be NULL if the handler does not support", now before this patch,
handlers available would be limited and their callbacks were fixed so
it was assumed that the code would not call the non-existent callback
which were NULL, from what I understand.

But now nothing stops the extension from giving a NULL callback, I
tried this with test_sync_handler: I set sync_unlinkfiletag to NULL,
queued an unlink request and ran CHECKPOINT. The checkpointer died
with SIGSEGV and the postmaster restarted all processes.

Even though this is documented in code and docs, a simple NULL check
before calling the handler could prevent a crash, could this be
worthwhile adding?

I can send a small incremental patch for this and would like for
further development of the patch.

Regards,
Yash Jadhav