Date: Fri, 18 Sep 2020 21:18:11 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Magnus Hagander <magnus@hagander.net>,
	Michael Paquier <michael@paquier.xyz>,
	Andres Freund <andres@anarazel.de>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Online checksums patch - once again
Message-ID: <20200919021811.GA30557@telsasoft.com>
References: 
 <CA+Tgmoa_WTE6WGn7P05TeOCRO82d+mVg+TaLSe0xjd56WhOQmA@mail.gmail.com>
 <CABUevEwns5iuvJ_kH_9KXNyo3Z8=RHB-DM1-TKP26yoVo0=xvg@mail.gmail.com>
 <CA+TgmoZrwxbra85d7tjgT9CmrT3H5iHGTi40A=6gcReN7WgnHg@mail.gmail.com>
 <CABUevEzZcmyOHXNV91BHpFmM3Bm5wKsZUcUOUVqsh-Or6EJpTA@mail.gmail.com>
 <CA+TgmoY61f+dywbWvFCfReJP-yO5ey7Liv44j6f6oWJCX=fbYg@mail.gmail.com>
 <A5CA3D81-9C8A-4D04-987D-1BEB0559B3D3@yesql.se>
 <CA+TgmoaryNACeLnLpfhx==gYy1+59VhMtrWspcZGay9eDvYHMw@mail.gmail.com>
 <AE4A785C-607C-4059-96DB-E4952E7449DA@yesql.se>
 <CA+TgmoZV+gvfQd6O6KXW=yYVC2nUafs6cgFP1ZC_+w2qyM6POg@mail.gmail.com>
 <85114708-887F-4A03-A57F-249F3E0009E3@yesql.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <85114708-887F-4A03-A57F-249F3E0009E3@yesql.se>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

+ * changed to "inprogress-off", the barrier for mvoving to "off" can be
moving

+ * When disabling checksums, data_checksums will be set of "inprogress-off"
set *to*

+	get_namespace_name(RelationGetNamespace(reln)), RelationGetRelationName(reln),

I think this palloc()s a new copy of the namespace every 100 blocks.
Better do it outside the loop.

+	{"inprogress-on", DATA_CHECKSUMS_INPROGRESS_ON, true},
+	{"inprogress-off", DATA_CHECKSUMS_INPROGRESS_OFF, true},

enabling / disabling ?

+typedef enum ChecksumType
+{
+	DATA_CHECKSUMS_OFF = 0,
+	DATA_CHECKSUMS_ON,
+	DATA_CHECKSUMS_INPROGRESS_ON,
+	DATA_CHECKSUMS_INPROGRESS_OFF
+}			ChecksumType;

Should this be an bitmask, maybe
DATA_CHECKSUMS_WRITE = 1 
DATA_CHECKSUMS_VERIFY = 2,

It occured to me that you could rephrase this patch as "Make checksum state
per-relation rather than cluster granularity".  That's currently an
implementation detail, but could be exposed as a feature.  That could be a
preliminary 0001 patch.  Half the existing patch would be 0002 "Allow
online enabling checksums for a given relation/database/cluster".  You might
save some of the existing effort of synchronize the cluster-wide checksum
state, since it doesn't need to be synchronized.  The "data_checksums" GUC
might be removed, or changed to an enum: on/off/per_relation.  Moving from
"per_relation" to "on" would be an optional metadata-only change, allowed only
when all rels in all dbs are checksumed.  I'm not sure if you'd even care about
temp tables, since "relhaschecksum" would be authoritative, rather than a
secondary bit only used during processing.

XLogHintBitIsNeeded() and DataChecksumsEnabled() would need to check
relhaschecksum, which tentatively seems possible.

I'm not sure if it's possible, but maybe pg_checksums would be able to skip
rels which had already been checksummed "online" (with an option to force
reprocessing).

Maybe some people would want (no) checksums on specific tables, and that could
eventually be implemented as 0003: "ALTER TABLE SET checksums=".  I'm thinking
of that being used immediately after an CREATE, but I suppose ON would cause
the backend to rewrite the table with checksums synchronously (not in the BGW),
either with AEL or by calling ProcessSingleRelationByOid().

-- 
Justin