Date: Thu, 30 Dec 2021 16:16:34 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	"houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
	Rahila Syed <rahilasyed90@gmail.com>,
	Peter Smith <smithpb2250@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: Column Filtering in Logical Replication
Message-ID: <20211230221634.GT24477@telsasoft.com>
References: <202112300015.sh7xv36pgao2@alvherre.pgsql>
 <202112302021.ca7ihogysgh3@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202112302021.ca7ihogysgh3@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20211230221634.GT24477%40telsasoft.com>
Precedence: bulk

> +	bool		am_partition = false;
>...
>	Assert(!isnull);
>	lrel->relkind = DatumGetChar(slot_getattr(slot, 3, &isnull));
>	Assert(!isnull);
> +	am_partition = DatumGetChar(slot_getattr(slot, 4, &isnull));

I think this needs to be GetBool.
You should Assert(!isnull) like the others.
Also, I think it doesn't need to be initialized to "false".

> +		/*
> +		 * Even if the user listed all columns in the column list, we cannot
> +		 * allow a column list to be specified when REPLICA IDENTITY is FULL;
> +		 * that would cause problems if a new column is added later, because
> +		 * that could would have to be included (because of being part of the

could would is wrong

> +	/*
> +	 * Translate list of columns to attnums. We prohibit system attributes and
> +	 * make sure there are no duplicate columns.
> +	 *
> +	 */

extraneous line

> +/*
> + * Gets a list of OIDs of all column-partial publications of the given
> + * relation, that is, those that specify a column list.

I would call this a "partial-column" publication.

> +					errmsg("cannot set REPLICA IDENTITY FULL when column-partial publications exist"));
> +	 * Check column-partial publications.  All publications have to include all

same

> +	/*
> +	 * Store the column names only if they are contained in column filter

period(.)

> +	 * LogicalRepRelation will only contain attributes corresponding to those
> +	 * specficied in column filters.

specified

> --- a/src/include/catalog/pg_publication_rel.h
> +++ b/src/include/catalog/pg_publication_rel.h
> @@ -31,6 +31,9 @@ CATALOG(pg_publication_rel,6106,PublicationRelRelationId)
>  	Oid			oid;			/* oid */
>  	Oid			prpubid BKI_LOOKUP(pg_publication); /* Oid of the publication */
>  	Oid			prrelid BKI_LOOKUP(pg_class);	/* Oid of the relation */
> +#ifdef CATALOG_VARLEN
> +	int2vector	prattrs;		/* Variable length field starts here */
> +#endif

The language in the pre-existing comments is better:
	/* variable-length fields start here */

> @@ -791,12 +875,13 @@ fetch_remote_table_info(char *nspname, char *relname,
>
>                 ExecClearTuple(slot);
>         }
> +
>         ExecDropSingleTupleTableSlot(slot);
> +       walrcv_clear_result(res);
> +       pfree(cmd.data);
>
>         lrel->natts = natt;
>
> -       walrcv_clear_result(res);
> -       pfree(cmd.data);
>  }

The blank line after "lrel->natts = natt;" should be removed.