Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation

On Thu, Oct 03, 2024 at 06:55:47AM -0700, Masahiko Sawada wrote:
> I've attached PoC patches for the idea Noah proposed. Newly created
> clusters unconditionally have default_char_signedness=true, and the
> only source of signedness=false is pg_upgrade. To update the
> signedness in the controlfile, pg_resetwal now has a new option
> --char-signedness, which is used by pg_upgrade internally. Feedback is
> very welcome.

Upthread, we discussed testability.  Does pg_resetwal facilitate all
appropriate testing, or do testing difficulties remain?

I reviewed these patches, finding only one non-cosmetic review comment.  Given
the PoC status, some of the observations below are likely ones you already
know or would have found before exiting PoC.

> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -27858,6 +27858,11 @@ acl      | {postgres=arwdDxtm/postgres,foo=r/postgres}
>         <entry><type>integer</type></entry>
>        </row>
>  
> +      <row>
> +       <entry><structfield>signed_char</structfield></entry>
> +       <entry><type>bool</type></entry>

s/signed_char/default_char_signedness/ to align with the naming below.

> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -4256,6 +4256,33 @@ WriteControlFile(void)

> +	 * Newly created database clusters unconditionally set the default char
> +	 * signedness to true. pg_upgrade changes this flag for clusters that were
> +	 * initialized on signedness=false platforms. As a result,
> +	 * signedness=false setting will become rare over time. will get rarer
> +	 * over time.

There's a redundant fragment of sentence.

> --- a/doc/src/sgml/ref/pg_resetwal.sgml
> +++ b/doc/src/sgml/ref/pg_resetwal.sgml
> @@ -171,6 +171,16 @@ PostgreSQL documentation
>    </para>
>  
>    <variablelist>
> +   <varlistentry>
> +    <term><option>--char-signedness=<replaceable class="parameter">option</replaceable></option></term>
> +    <listitem>
> +     <para>
> +      Manually set the default char signedness. Possible values are
> +      <literal>signed</literal> and <literal>unsigned</literal>.
> +     </para>
> +    </listitem>
> +   </varlistentry>

This needs more docs, like its <varlistentry> neighbors have.  First, see the
point below about the pg_upgrade docs.

> --- a/src/bin/pg_resetwal/pg_resetwal.c
> +++ b/src/bin/pg_resetwal/pg_resetwal.c
> @@ -1189,6 +1213,7 @@ usage(void)
>  	printf(_("  -u, --oldest-transaction-id=XID  set oldest transaction ID\n"));
>  	printf(_("  -x, --next-transaction-id=XID    set next transaction ID\n"));
>  	printf(_("      --wal-segsize=SIZE           size of WAL segments, in megabytes\n"));
> +	printf(_("      --char-signedness=OPTION     set char signedness to \"signed\"  or \"unsigned\"\n"));

This help output alphabetizes by short option, with the one non-short option
at the end.  So I'd also alphabetize among the non-short options, specifically
putting the new line before --wal-segsize.

> source cluster was initialized on the same platform where pg_upgrae is

Typo "pg_upgrade"

> --- a/src/bin/pg_upgrade/controldata.c
> +++ b/src/bin/pg_upgrade/controldata.c
> @@ -62,6 +62,7 @@ get_control_data(ClusterInfo *cluster)
>  	bool		got_date_is_int = false;
>  	bool		got_data_checksum_version = false;
>  	bool		got_cluster_state = false;
> +	bool		got_default_char_signedness = false;
>  	char	   *lc_collate = NULL;
>  	char	   *lc_ctype = NULL;
>  	char	   *lc_monetary = NULL;
> @@ -206,6 +207,13 @@ get_control_data(ClusterInfo *cluster)
>  		got_data_checksum_version = true;
>  	}
>  
> +	/* Only in <= 17 */
> +	if (GET_MAJOR_VERSION(cluster->major_version) <= 1700)
> +	{
> +		cluster->controldata.default_char_signedness = true;

If anything reads the "true" stored here, that would be a bug.  Is there a
reasonable way do one or two of the following?

1. not initialize it at all
2. store something like -1 instead
3. add a comment that it's never read

> +		got_default_char_signedness = true;
> +	}

I wouldn't say we "got" this.  I'd handle this more like got_oldestmulti,
where we know !got_oldestmulti is okay until MULTIXACT_FORMATCHANGE_CAT_VER.
Maybe also replace the "<= 1700" checks with catversion checks.

> +
>  	/* we have the result of cmd in "output". so parse it line by line now */
>  	while (fgets(bufin, sizeof(bufin), output))
>  	{
> @@ -501,6 +509,17 @@ get_control_data(ClusterInfo *cluster)
>  			cluster->controldata.data_checksum_version = str2uint(p);
>  			got_data_checksum_version = true;
>  		}
> +		else if ((p = strstr(bufin, "char data signedness:")) != NULL)
> +		{
> +			p = strchr(p, ':');
> +
> +			if (p == NULL || strlen(p) <= 1)
> +				pg_fatal("%d: controldata retrieval problem", __LINE__);
> +
> +			p++;				/* remove ':' char */
> +			cluster->controldata.default_char_signedness = strstr(p, "signed") != NULL;

Won't this match "unsigned", too?  (This is my only non-cosmetic review
comment.)  I'd strcmp for the two expected values.  If neither matches, report
the pg_fatal "retrieval problem".

> --- a/doc/src/sgml/ref/pgupgrade.sgml
> +++ b/doc/src/sgml/ref/pgupgrade.sgml
> @@ -276,6 +276,24 @@ PostgreSQL documentation
>        </listitem>
>       </varlistentry>
>  
> +     <varlistentry>
> +      <term><option>--set-char-signedness=</option><replaceable>option</replaceable></term>
> +      <listitem>
> +       <para>
> +        Manually set the default char signedness of new clusters. Possible values
> +        are <literal>signed</literal> and <literal>unsigned</literal>.
> +       </para>
> +       <para>
> +        The signedness of the 'char' type in C is implementation-dependent. For instance,
> +        'signed char' is used by default on x86 CPUs, while 'unsigned char' is used on aarch
> +        CPUs. <application>pg_upgrade</application> automatically inherits the char
> +        signedness from the old cluster. This option is useful for upgrading the cluster
> +        that user knew they copied it to a platform having different char signedness
> +        (e.g. from x86 to aarch).

That last sentence would need some grammar help.  Instead, let's rewrite this
paragraph to assume the reader is not a C programmer.  Focus on what such a
reader should do to choose what argument, if any, to pass.

Our docs haven't used the term "aarch".  This applies to 32-bit ARM in
addition to 64-bit ARM.  Hence, I'd write the term "ARM" instead.

> --- a/src/bin/pg_upgrade/option.c
> +++ b/src/bin/pg_upgrade/option.c
> @@ -307,6 +316,7 @@ usage(void)
>  	printf(_("  --copy                        copy files to new cluster (default)\n"));
>  	printf(_("  --copy-file-range             copy files to new cluster with copy_file_range\n"));
>  	printf(_("  --sync-method=METHOD          set method for syncing files to disk\n"));
> +	printf(_("  --set-char-signedness=OPTION  set new cluster char signedness to \"signed\" or \"unsigned\""));

Move this up one line, to preserve alphabetization.

> --- a/src/backend/storage/lmgr/predicate.c
> +++ b/src/backend/storage/lmgr/predicate.c
> @@ -2588,6 +2588,11 @@ PredicateLockPage(Relation relation, BlockNumber blkno, Snapshot snapshot)
>  	if (!SerializationNeededForRead(relation, snapshot))
>  		return;
>  
> +	ereport(LOG, (errmsg("predicate %s blk %u",
> +						 RelationGetRelationName(relation),
> +						 blkno),
> +				  errbacktrace()));

Revert this file.

Thanks,
nm