MIME-Version: 1.0
References: <CAAWbhmiRRJDzbA4fv0vAViu_wfoeYqtT+VYOOu7845WO-yT35w@mail.gmail.com>
 <CAAWbhmiQWEGv73En15WRJ5PmhbwwTdBxMiPxunGghW0G-WVAYg@mail.gmail.com>
 <CAJ7c6TO+6hs1sMJ5-Qs7DC5HyhhN+hkLU0D8Ci_F-5ruBaHNog@mail.gmail.com>
 <CAAWbhmha62Vh9MtsJ3zxRKRQiVSoyxb6sKHzLCRWsTzqwM0bbg@mail.gmail.com>
 <f6478284-8556-6991-ee9d-ae4b72813f0e@timescale.com> <dc57f088-039b-7a71-8f4c-082ef106246e@timescale.com>
 <CAHut+PuJq9s8QZ2pn2jKERWr3wa2ABT6McOxR=F-1+cJHtKSiQ@mail.gmail.com>
 <CAAWbhmi_sTjbSd-RYSC08DHNbXy9QG1FoHT+qGYxNwoO+MSSDg@mail.gmail.com>
 <13971415-416b-1e98-2fe5-ad394c6ddc2c@timescale.com> <7eca2186-5da7-9e3c-6faf-ced69536031c@timescale.com>
In-Reply-To: <7eca2186-5da7-9e3c-6faf-ced69536031c@timescale.com>
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Wed, 19 Jul 2023 14:54:53 +0300
Message-ID: <CAJ7c6TNRk96aTLyBTkcyxLwerHX6wB-fn7SW6ZUjY9V+q_aggg@mail.gmail.com>
Subject: Re: RFC: logical publication via inheritance root?
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Jacob Champion <jchampion@timescale.com>, Peter Smith <smithpb2250@gmail.com>, 
	Amit Kapila <amit.kapila16@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAJ7c6TNRk96aTLyBTkcyxLwerHX6wB-fn7SW6ZUjY9V%2Bq_aggg%40mail.gmail.com>
Precedence: bulk

Hi,

> v3 also fixes a nasty uninitialized stack variable, along with a bad
> collation assumption I made.

I decided to take a closer look at 0001.

Since pg_get_relation_publishing_info() is exposed to the users I
think it should be described in a bit more detail than:

```
+  descr => 'get information on how a relation will be published via a
list of publications',
```

This description in \df+ output doesn't seem to be particularly
useful. Also the function should be documented. In order to accomplish
all this it could make sense to reconsider the signature of the
function and/or split it into several separate functions.

The volatility is declared as STABLE. This is probably correct. At
least at first glance I don't see any calls of VOLATILE functions and
off the top of my head can't give an example when it will not behave
as STABLE. This being said, a second opinion would be appreciated.

process_relation_publications() misses a brief comment before the
declaration. What are the arguments, what is the return value, are
there any pre/postconditions (locks, memory), etc.

Otherwise 0001 is in a decent shape, it passes make
installcheck-world, etc. I would suggest focusing on delivering this
part, assuming there will be no push-back to the refactorings and
slight test improvements. If 0002 could be further decomposed into
separate iterative improvements this could be helpful.

-- 
Best regards,
Aleksander Alekseev