Date: Tue, 20 Jul 2021 20:14:17 -0400
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Rahila Syed <rahilasyed90@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Peter Smith <smithpb2250@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Column Filtering in Logical Replication
Message-ID: <202107210014.py4k662aarq5@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAH2L28sY8031_udZBf6nTJwY01KPnSdPvWrPYWsE4c2=2RN8CQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202107210014.py4k662aarq5%40alvherre.pgsql>
Precedence: bulk

Hello,

I think this looks good regarding the PublicationRelationInfo API that was
discussed.

Looking at OpenTableList(), I think you forgot to update the comment --
it says "open relations specified by a RangeVar list", but the list is
now of PublicationTable.  Also I think it would be good to say that the
returned tables are PublicationRelationInfo, maybe such as "In the
returned list of PublicationRelationInfo, the tables are locked ..."

In AlterPublicationTables() I was confused by some code that seemed
commented a bit too verbosely (for a moment I thought the whole list was
being copied into a different format).  May I suggest something more
compact like

			/* Not yet in list; open it and add it to the list */
			if (!found)
			{
				Relation	oldrel;
				PublicationRelationInfo *pubrel;
			   
				oldrel = table_open(oldrelid, ShareUpdateExclusiveLock);

				/* Wrap it in PublicationRelationInfo */
				pubrel = palloc(sizeof(PublicationRelationInfo));
				pubrel->relation = oldrel;
				pubrel->relid = oldrelid;
				pubrel->columns = NIL;		/* not needed */

				delrels = lappend(delrels, pubrel);
			}

Thanks!

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/