Hi,
On Thu, Apr 13, 2023 at 03:26:56PM +1000, Peter Smith wrote:
>
> 1.
> All the comments look alike, so it is hard to know what is going on.
> If each of the main test parts could be highlighted then the test code
> would be easier to read IMO.
>
> Something like below:
> [...]
I added a bit more comments about what's is being tested. I'm not sure that a
big TEST CASE prefix is necessary, as it's not really multiple separated test
cases and other stuff can be tested in between. Also AFAICT no other TAP test
current needs this kind of banner, even if they're testing more complex
scenario.
> 2.
> +# replication origin's remote_lsn isn't set if not data is replicated after the
> +# initial sync
>
> wording:
> /if not data is replicated/if data is not replicated/
I actually mean "if no data", which is a bit different than what you suggest.
Fixed.
> 3.
> # Make sure the replication origin is set
>
> I was not sure if all of the SELECT COUNT(*) checking is needed
> because it just seems normal pub/sub functionality. There is no
> pg_upgrade happening, so really it seemed the purpose of this part was
> mainly to set the origin so that it will not be a blocker for
> ready-state tests that follow this code. Maybe this can just be
> incorporated into the following test part.
Since this patch is transferring internal details about subscriptions I prefer
to be thorough about what is tested, when data is actually being replicated and
so on so if something is broken (relation added to the wrong subscription,
wrong oid or something) it should immediately show what's happening.
> 4a.
> TBH, I felt it might be easier to follow if the SQL was checking for
> WHERE (text = "while old_sub is down") etc, rather than just using
> SELECT COUNT(*), and then trusting the comments to describe what the
> different counts mean.
I prefer the plain count as it's a simple way to make sure that the state is
exactly what's wanted. If for some reason the patch leads to previous row
being replicated again, such a test wouldn't reveal it. Sure, it could be
broken enough so that one old row is replicated twice and the new row isn't
replicated, but it seems so unlikely that I don't think that testing the whole
table content is necessary.
> 4b.
> All these messages like "Table t1 should still have 2 rows on the new
> subscriber" don't seem very helpful. e.g. They are not saying anything
> about WHAT this is testing or WHY it should still have 2 rows.
I don't think that those messages are supposed to say what or why something is
tested, just give a quick context / reference on the test in case it's broken.
The comments are there to explain in more details what is tested and/or why.
> 5.
> # Refresh the subscription, only the missing row on t2 show be replicated
>
> /show/should/
Fixed.
