errhint in a similar way. There was no reply to Amit's
idea, so it's not clear whether it's an accidental omission from your
v2 patch or not.
--
[1]
https://www.postgresql.org/message-id/CAA4eK1J11phiaoCOmsjNqPZ9BOWyLXYrfgrm5vU2uCFPF2kN1Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
adding similar new double
quotes to all other VERBOSE logging in your patch.
e.g. see get_logical_slot_infos:
pg_log(PG_VERBOSE, "Database: %s", pDbInfo->db_name);
--
Kind Regards,
Peter Smith.
Fujitsu Australia
7mNSLgA%40mail.gmail.com
[2] Kuroda-san reply to my v11 review -
https://www.postgresql.org/message-id/TYAPR01MB5866BD618DEE62AF1836E612F5749%40TYAPR01MB5866.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, May 11, 2023 at 6:30 PM Daniel Gustafsson wrote:
>
> > On 11 May 2023, at 07:41, Peter Smith wrote:
>
> > While reviewing another patch for the file info.c, I noticed some
> > misplaced colon (':') in the verbose logs for print_rel_infos().
&
Hi --
While reviewing another patch for the file info.c, I noticed some
misplaced colon (':') in the verbose logs for print_rel_infos().
PSA patch to fix it.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-pg_upgrade-typo-in-verbose-log.patch
Description: Binary data
Hi.
While reviewing another patch to the file info.c, I noticed there seem
to be some unnecessary calls to strlen(query) in get_rel_infos()
function.
i.e. The query is explicitly initialized to an empty string
immediately prior, so why the strlen?
PSA patch for this.
--
Kind Regards,
Peter
same info?
==
.../pg_upgrade/t/003_logical_replication_slots.pl
5.
+# Preparations for the subsequent test. The case max_replication_slots is set
+# to 0 is prohibit.
/prohibit/prohibited/
--
[1] My v10 review -
https://www.postgresql.org/message-id/CAHut%2BPtpQaKVfqr-8KUtGZqei1C9gWF0%2BY8
On Mon, Apr 24, 2023 at 4:19 PM Julien Rouhaud wrote:
>
> 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 high
e. Probably
they should be consistent.
~
13b.
Something seems amiss. Here the is_error is assigned true; But later
when you test is_error that is for logging the ready-state problem.
Isn't there another missing pg_fatal for this invalid remote_lsn case?
==
src/bin/pg_upgrade/option.c
1
ation_slot('test_slot',
'test_decoding', false, true);
2023-05-09 12:19:25.335 AEST [32564] LOG: received immediate shutdown request
2023-05-09 12:19:25.337 AEST [32564] LOG: database system is shut down
~
Is it a bug? Or, if I am doing something wrong please let me know how
to run the test.
~~~
19.
+# Clean up
+rmtree($new_node->data_dir . "/pg_upgrade_output.d");
+$new_node->append_conf('postgresql.conf', "wal_level = 'logical'");
+$new_node->append_conf('postgresql.conf', "max_replication_slots = 0");
I think the last 2 lines are not "clean up". They are preparations for
the subsequent test, so maybe they should be commented as such.
~~~
20.
+# Clean up
+rmtree($new_node->data_dir . "/pg_upgrade_output.d");
+$new_node->append_conf('postgresql.conf', "max_replication_slots = 10");
I think the last line is not "clean up". It is preparation for the
subsequent test, so maybe it should be commented as such.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, May 8, 2023 at 2:05 PM Michael Paquier wrote:
>
> On Mon, May 08, 2023 at 01:57:33PM +1000, Peter Smith wrote:
> > I agree. Do you want me to make a new v4 patch to also do that, or
> > will you handle them in passing?
>
> I'll just go handle them on the wa
On Mon, May 8, 2023 at 1:09 PM Michael Paquier wrote:
>
> On Mon, May 08, 2023 at 10:29:50AM +1000, Peter Smith wrote:
> > Thanks for giving some feedback on my patch.
>
> Looks OK.
>
> While on it, looking at logical-replication.sgml, it seems to me that
> these two
lot by executing ALTER SUBSCRIPTION ... SET (slot_name =
NONE).
SUGGESTION
To proceed in this situation, first DISABLE the subscription, and then
disassociate it from the replication slot by executing ALTER
SUBSCRIPTION ... SET (slot_name = NONE).
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sat, May 6, 2023 at 5:28 AM David Zhang wrote:
>
> On 2023-03-16 4:46 p.m., Peter Smith wrote:
> > A rebase was needed due to the recent REPLICA IDENTITY push [1].
> >
> > PSA v2.
> >
> >
> > - A published table must have a replica identity
&
/regress/expected/psql.out
38.
IMO the column output is not in a good order. See review comments for describe.c
==
src/test/regress/expected/publication.out
39.
I previously posted a review comment about some missing test cases
([1] #21). The reply was "they will be added in later patches". I
think it's better to put the relevant tests in the same patch that
introduced the functionality.
--
My previous review posts for this patch
[1]
https://www.postgresql.org/message-id/CAHut%2BPtzpuuRFrLnjkQePq296ip_0WfmQ4CtydM9JDR6gEf%3DQw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPtMkVoweJrd%3DSLw7BfpW883skasdnemoj4N19NnyjrT3Q%40mail.gmail.com
[3]
https://www.postgresql.org/message-id/cahut+pug8j8ua5v-f-o4tczhvfswgg1b8ql+ezo0hjwweey...@mail.gmail.com
[4]
https://www.postgresql.org/message-id/CAHut%2BPtOODRybaptKRKUWZnGw-PZuLF2BxaitnMSNeAiU8-yPg%40mail.gmail.com
[5] Houz replies to my previous review comments -
https://www.postgresql.org/message-id/OS0PR01MB571690B2DB46B1C4AF61D184946F9%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
>exprstate, 0, sizeof(entry->exprstate));
Continually adding to these assignment has got a bit out of control...
IMO the code now would be better written as:
memset(entry->pubactions, 0, sizeof(entry->pubactions));
And doing this would also be consistent with the similar code for
entry->exprstate (just a couple of lines below here).
~~~
35. get_rel_sync_entry
entry->pubactions.pubupdate = false;
entry->pubactions.pubdelete = false;
entry->pubactions.pubtruncate = false;
+ entry->pubactions.pubddl_table = false;
(same as above review comment #35)
IMO all this should be written more simply as:
memset(entry->pubactions, 0, sizeof(entry->pubactions));
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPtzpuuRFrLnjkQePq296ip_0WfmQ4CtydM9JDR6gEf%3DQw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPtMkVoweJrd%3DSLw7BfpW883skasdnemoj4N19NnyjrT3Q%40mail.gmail.com
[3]
https://www.postgresql.org/message-id/cahut+pug8j8ua5v-f-o4tczhvfswgg1b8ql+ezo0hjwweey...@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
sizeof(Oid));
+ data += sizeof(Oid);
+ memcpy(data, &change->data.ddl.cmdtype, sizeof(DeparsedCommandType));
+ data += sizeof(int);
+ memcpy(data, change->data.ddl.prefix, prefix_size);
+ data += prefix_size;
Isn't it better to use sizeof(DeparsedCommandType) instead of
sizeof(int)
X" (e.g. like here
https://www.postgresql.org/about/featurematrix/).
~~~
16.
+
+ The DDL deparser utility is invoked during the replication of
DDLs. The DDL
+ deparser is capable of converting a DDL command into formatted
JSON blob, with
+ the necessary information to reconstruct the DDL commands at
the destination. The
+ benefit of using the deparser output compared to the original
command string
+ includes:
"The benefit ... includes:" --> "The benefits ... include:"
~~~
17.
+ The DDL deparser exposes two SQL functions:
+
I imagine that these SQL functions should be documented elsewhere as well.
Possibly on this page?
https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-REPLICATION
--
Kind Regards,
Peter Smith.
Fujitsu Australia
dlreplok */
+v(CMDTAG_UNKNOWN, "???", false, false, false, false)
Although these are not strictly the same as the PG_CMDTAG macro arg
names, it might be better in this comment to call this "ddl_repl_ok"
instead of "ddlreplok" for consistency with the rest of the comment.
==
src/test/regress/expected/publication.out
21.
The \dRp+ now exposes a new column called "Table DDLS"
I expected to see some tests for t/f values but I did not find any
test where the expected output for this column was 't'.
==
src/tools/pgindent/typedefs.list
22.
LogicalDecodeCommitPreparedCB
+LogicalDecodeDDLMessageCB
+LogicalDecodeStreamDDLMessageCB
LogicalDecodeFilterByOriginCB
The alphabetical order is not correct for LogicalDecodeStreamDDLMessageCB
~~~
23.
ReorderBufferCommitPreparedCB
+ReorderBufferDDLMessageCB
+ReorderBufferStreamDDLMessageCB
ReorderBufferDiskChange
The alphabetical order is not correct for ReorderBufferStreamDDLMessageCB
--
Kind Regards,
Peter Smith.
Fujitsu Australia
PLICATION SLOT (ID %d NAME
%s)"?
==
.../pg_upgrade/t/003_logical_replication.pl
3.
Should the name of this TAP test file really be 03_logical_replication_slots.pl?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
out WHAT this is testing or WHY it should still have 2 rows.
~~~
5.
# Refresh the subscription, only the missing row on t2 show be replicated
/show/should/
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
the message can include the names
of the failing subscription and/or the relation that was in the wrong
state. Maybe that means moving all this checking logic into the
pg_dump code?
==
src/bin/pg_upgrade/option.c
16. parseCommandLine
user_opts.transfer_mode = TRANSFER_MODE_COPY;
+ user_opt
looks a bit strange that you used it for only some of
the columns but not others.
~~~
3.
+
+ /* FIXME: force dumping */
+ slotinfo[i].dobj.dump = DUMP_COMPONENT_ALL;
Why the "FIXME" here? Are you intending to replace this code with
something else?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
adding more checks or an assert
here because... */
OTOH, "Note" is just for highlighting why something is the way it is,
but with no implication that it should be revisited/changed in the
future.
e.g.
/* Note: We deliberately do not test the state here because... */
/* Note: This memory must be zeroed because... */
/* Note: This string has no '\0' terminator so... */
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ot' but
AFAIK the proper option name everywhere else in this patch is
'--include-logical-replication-slots' (with the 's')
~
5b.
I'm not sure that "pg_upgrade for new publisher" makes sense.
It's more like "pg_upgrade of old publisher", or simply "pg_upgrade of
publisher"
--
[1]
https://www.postgresql.org/message-id/TYCPR01MB5870E212F5012FD6272CE1E3F5969%40TYCPR01MB5870.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Apr 11, 2023 at 4:36 PM Thomas Munro wrote:
>
> On Tue, Apr 11, 2023 at 6:16 PM Peter Smith wrote:
> > cfbot [1] is listing some already committed patches under the "Needs
> > Review" category. For example here are some of mine [1][2]. And
> > becau
://cfbot.cputube.org/next.html
[2] https://commitfest.postgresql.org/43/4246/
[3] https://commitfest.postgresql.org/43/4266/
Kind Regards,
Peter Smith.
Fujitsu Australia
SK)
probably this should be called DB_DUMP_SLOTS_FILE_MASK.
~
20b.
Because the content of this dump/restore file is SQL (not custom
binary) wouldn't a filename suffix ".sql" be better?
==
.../pg_upgrade/t/003_logical_replication.pl
21.
Some parts (formatting, comments, etc) in thi
Thanks for pushing.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
~~
PSA a tiny patch to fix that.
--
[1]
https://github.com/postgres/postgres/commit/1e10d49b65d6c26c61fee07999e4cd59eab2b765
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-comment-typo.patch
Description: Binary data
; option.
~~
AFAICT the associated tab-complete code was accidentally omitted.
PSA patches to add those tab completions.
--
[1]
https://github.com/postgres/postgres/commit/c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6
[2]
https://github.com/postgres/postgres/commit/482675987bcdffb390ae735cfd5
FYI, the WIP patch does not seem to apply cleanly anymore using the latest HEAD.
See the cfbot rebase logs [1].
--
[1] http://cfbot.cputube.org/patch_42_4225.log
Kind Regards,
Peter Smith.
Fujitsu Australia
users are self-evident from your test cases.
--
[1]
https://docs.timescale.com/use-timescale/latest/hypertables/about-hypertables/
[2] https://www.crunchydata.com/blog/native-partitioning-with-postgres
[3] https://github.com/pgpartman/pg_partman
Kind Regards,
Peter Smith.
Fujitsu Australia
ON'T
want to get duplicated data from other partitions (because you already
knew about those ones -- like your example does).
So, I am not sure what the answer is, or maybe there isn't one.
At least, we need to check there are sufficient "BE CAREFUL" warnings
in the documentation for scenarios like this.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
atches are the latest, you will be easily able to unambiguously
refer to them by name in subsequent posts, and when copied to your
local computer they won't clash with any older copied patches.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Hou-san,
I looked again at v4-0001.
On Thu, Mar 30, 2023 at 2:01 PM houzj.f...@fujitsu.com
wrote:
>
> On Thursday, March 30, 2023 9:15 AM Peter Smith wrote:
> >
...
> >
> > 2.
> > /* Convert tuple if needed. */
> > if (relentry-> attrmap)
> > {
"ready for committer"
--
[1]
https://www.postgresql.org/message-id/TYAPR01MB5866879FFE5E0A2726244216F5889%40TYAPR01MB5866.jpnprd01.prod.outlook.com
[2] https://commitfest.postgresql.org/43/4260/
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Mar 30, 2023 at 12:57 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear hackers,
>
> This thread is motivated from [1]. This patch adds some links that refer
> publication options.
>
Is this the correct attachment?
------
Kind Regards,
Peter Smith,
Fujitsu Australia
UPDATE
or DELETE, so the code would be better to include a sanity Assert.
SUGGESTION
if (change->data.tp.oldtuple)
{
Assert(action == REORDER_BUFFER_CHANGE_UPDATE || action ==
REORDER_BUFFER_CHANGE_DELETE);
...
}
==
6.
I suggest moving the "change->data.tp.oldtuple" check before the
"change->data.tp.newtuple" check. I don't think it makes any
difference, but it seems more natural IMO to have old before new.
--
Kind Regards,
Peter Smith
e
a default tableRow[2] that is valid for one case and overwrite it only
for the other case, instead of overwriting it in 2 places.
YMMV.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Thanks for this patch.
v5-0001 looks good to me.
v5-0002 looks good to me.
I've marked the CF entry [1] as "ready for committer".
--
[1] https://commitfest.postgresql.org/43/4256/
Kind Regards,
Peter Smith.
Fujitsu Australia
>
- originate (see CREATE
PUBLICATION).
+ originate (see publish_via_partition_root
+ of CREATE PUBLICATION).
Hmm, my above-suggested wording was “publish_via_partition_root
parameter “ but it seems you (accidentally?) omitted the word
“parameter”.
Otherwise, the patch v4-0002 also LGTM
--
Kind Regards,
Peter Smith.
Fujitsu Australia
sher or the subscriber.
There is a cut/paste typo here -- it renders badly with "FOR TABLES IN
SCHEMA" appearing 2x.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Pvzo6%3DKKLqMR6-mAQdM%2Bj_dse0eUreGmrFouL7gbLbv2w%40mail.gmail.com#7da8d0e3b73096375847c16c856b4aed
Kind Regards,
Peter Smith.
Fujitsu Australia
:
+# tap_pub_parent_sync filter is: (a > 15)
+# tap_pub_child_sync filter is: (a < 15)
Maybe wrapping can be improved in the above comment and a full stop
added to the first sentence.
Otherwise, I have no more comments for v24.
--
Kind Regards,
Peter Smith
o the "publish" parameter.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
y "streaming option").
==
doc/src/sgml/ref/alter_subscription.sgml
The SKIP part says "... enabling two_phase on subscriber.". I thought
there could be a link for "two_phase" here (also "on subscriber" -->
"on the subscriber").
--
Kind Regards,
Peter Smith.
Fujitsu Australia
esting normal logical replication behavior.
#
~
I think you should add a couple of INSERTS for the newly added table/s
also. IMO it is not only better for test completeness, but it causes
readers to question why there are INSERTS for every other table except
these ones.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
be
+ automatically enabled by the subscriber if the subscription had been
+ originally created with two_phase = true option.
SUGGESTION (per my general comment about links/fonts)
two_phase
option
--
Kind Regards,
Peter Smith.
Fujitsu Australia
.
--
[1] https://www.postgresql.org/docs/devel/sql-createsubscription.html
[2] https://www.postgresql.org/message-id/2106581.1679610361%40sss.pgh.pa.us
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Wang-san. I looked at the v21-0001 patch.
I don't have any new review comments -- only follow-ups for some of my
previous v20 comments that were rejected.
On Thu, Mar 23, 2023 at 8:11 PM wangw.f...@fujitsu.com
wrote:
>
> On Thu, Mar 23, 2023 at 12:27 PM Peter Smith wrote:
> &
On Fri, Mar 24, 2023 at 9:26 AM Tom Lane wrote:
>
> Peter Smith writes:
> > While reviewing another thread [1] I could not find the function
> > 'pg_get_publication_tables' described anywhere in the PG
> > documentation.
> > Should it be mentioned somewh
On Thu, Mar 23, 2023 at 5:51 PM Amit Kapila wrote:
>
> On Thu, Mar 23, 2023 at 9:57 AM Peter Smith wrote:
> >
> > Here are some review comments for patch v20-0001.
> >
> > ==
> > General.
> >
> > 1.
> > That function 'pg_get_publ
tted for some
reason?
Thanks.
--
[1]
https://www.postgresql.org/message-id/CAA4eK1KrzTOYsuCzz6fxRed37C6MfHE1t9kyrM5B4m9ToqKWrQ%40mail.gmail.com
[2]
https://www.postgresql.org/docs/devel/functions-info.html#FUNCTIONS-INFO-CATALOG-TABLE
Kind Regards,
Peter Smith.
Fujitsu Australia
uot; part are not necessary.
~~
2.
extern void LogReplicationSlotAquired(bool is_physical, char *slotname);
extern void LogReplicationSlotReleased(bool is_physical, char *slotname);
The "char *slotname" params of those helper functions should probably
be declared and defined as "const char *slotname".
~~
Otherwise, from a code review perspective the patch v8 LGTM.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
some central point to
comment the intent of this logging? Feel free to take or leave this
code suggestion.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
BLICATION
pub_root_true;
+ CREATE
~
(This is simlar to the previous review comment #7 above)
Won't it be a better test of the "At least one" code when only the
publication of partition (test_root_1) is using "WITH
(publish_via_partition_root = true)".
e.g
CREATE PUBLICATION pub_root_true_1 FOR TABLE test_root (a);
CREATE PUBLICATION pub_root_true_2 FOR TABLE test_root_1 (a, b) WITH
(publish_via_partition_root = true);
--
[1]
https://www.postgresql.org/docs/devel/functions-info.html#FUNCTIONS-INFO-CATALOG-TABLE
[2]
https://www.postgresql.org/message-id/OS3PR01MB6275FB5397C6A647F262A3A69E009%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
tringInfoData pub_names;
+ initStringInfo(&pub_names);
+ get_publications_str(publications, &pub_names, true);
+ pfree(pub_names.data);
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPuNsvO9o9XzeJuSLsAsndgCKVphDPBRqYuOTy2bR28E%2Bg%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Thanks for all the patch updates. Patch v19 LGTM.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
nt PRIMARY KEY
);
INSERT INTO public.test_mismatching_types (a)
VALUES (1), (2);
));
# Refresh the publication to trigger the tablesync
$node_subscriber->safe_psql(
'postgres', qq(
ALTER SUBSCRIPTION tsub REFRESH PUBLICATION;
));
--
Kind Regards,
Peter Smith.
Fujitsu Australia
edundant "== true" part.
SUGGESTION
if (nulls[2])
==
src/include/catalog/pg_proc.dat
4.
+{ oid => '6119',
+ descr => 'get information of the tables in the given publication array',
Should that be worded in a way to make it more clear that the
"publication array" is really an "array of publication names"?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sat, Mar 18, 2023 at 8:47 AM Peter Smith wrote:
>
> The build-farm was OK for the last 18hrs after this push, except there
> was one error on mamba [1] in test-decoding-check.
>
> This patch did change the test_decoding.c file, so it seems an
> unlikely coincidence, but O
On Sun, Mar 19, 2023 at 2:00 AM Alexander Lakhin wrote:
>
> Hi,
>
> 18.03.2023 07:26, Tom Lane wrote:
>
> Amit Kapila writes:
>
> Peter Smith has recently reported a BF failure [1]. AFAICS, the call
> stack of failure [2] is as follows:
>
> Note the assert
n't needlessly attempt to relaunch until you have set
binary=false and then re-enabled the subscription.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
able
synchronization will use the same format as specified by the
subscription binary mode. If the publisher is before v16, then any
initial table synchronization will use text format regardless of the
subscription binary mode.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
oblem here but nowhere else.
Although, mamba has since passed again since that failure.
Any thoughts?
--
[1]
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba&dt=2023-03-17%2005%3A36%3A10
Kind Regards,
Peter Smith.
Fujitsu Australia
nup:
if (RelationIsValid(ancestor))
{
RelationClose(ancestor);
~
Since you've introduced a new label 'cleanup:' then IMO you can remove
that old comment "/* Cleanup */".
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Mar 17, 2023 at 12:20 AM Melih Mutlu wrote:
>
> Hi,
>
> Please see the attached v16.
>
> Peter Smith , 16 Mar 2023 Per, 03:03 tarihinde şunu
> yazdı:
>>
>> Here are some review comments for v15-0001
>
>
> I applied your comments in the updated pa
searched the TAP tests I could not find any existing tests
that check the combination of
- different column orders
- CREATE SUBSCRIPTION with parameters binary=true and copy_data=true
So there seemed to be a gap in the test coverage, which is why I suggested it.
I guess that test was not strictly
A rebase was needed due to the recent REPLICA IDENTITY push [1].
PSA v2.
--
[1]
https://github.com/postgres/postgres/commit/89e46da5e511a6970e26a020f265c9fb4b72b1d2
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0001-PGDOCS-replica-identity-quotes.patch
Description: Binary data
(or maybe a lot?) of common
internal code with the table DDL replication work, but OTOH an
auto-create feature for subscriber tables seems like it might be a
useful feature to have regardless of the value of the publication
'ddl' parameter.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
> +(\
> + rbtxn_is_subtxn(txn) ? (txn)->toptxn : (txn)\
> +)
>
> We need a whitespace before backslashes.
>
Thanks for your interest in my patch.
PSA v4 which addresses both of your review comments.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v4-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch
Description: Binary data
On Wed, Mar 15, 2023 at 5:31 PM Amit Kapila wrote:
>
> On Wed, Mar 15, 2023 at 11:52 AM Peter Smith wrote:
> >
> > ==
> > src/backend/replication/logical/tablesync.c
> >
> > 5.
> > +
> > + /*
> > + * If the publisher version is earlier
-
~
# --
# Test different column orders on pub/sub tables
# --
~
# -
# Test mismatched column types with/without binary mode
# -
--
Kind Regards,
Peter Smith.
Fujitsu Australia
esql.org/message-id/CAHut%2BPsAS8HpjdbDv%2BRM-YUJaLO0UC3f5be%2BqN296%2BGrewsGXg%40mail.gmail.com
[2] pg docs COPY - https://www.postgresql.org/docs/current/sql-copy.html
[3] pg docs COPY v9.0 - https://www.postgresql.org/docs/9.0/sql-copy.html
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Mar 14, 2023 at 10:33 PM vignesh C wrote:
>
> On Tue, 14 Mar 2023 at 12:37, Peter Smith wrote:
> >
> > Thanks for the review!
> >
> > On Mon, Mar 13, 2023 at 6:19 PM vignesh C wrote:
> > ...
> > > Few comments:
> > > 1) Can we move
On Tue, Mar 14, 2023 at 10:43 PM Amit Kapila wrote:
>
> On Tue, Mar 14, 2023 at 12:37 PM Peter Smith wrote:
> >
> > Thanks for the review!
> >
> > On Mon, Mar 13, 2023 at 6:19 PM vignesh C wrote:
> > ...
> >
> > > 4) We check if txn->toptxn
gle place to be the odd one out. TBH, I don't think 1 extra check
is of any significance, but it is not a problem to change like you
suggested if other people also want it done that way.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
v2-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch
Description: Binary data
1|2
3|4', 'check synced data on subscriber for different column order and
binary = true');
#
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Mar 14, 2023 at 11:06 AM Peter Smith wrote:
>
> Here are some review comments for patch v12-0001
>
> ==
> General
>
> 1.
> There is no new test code. Are we sure that there are already
> sufficient TAP tests doing binary testing with/without copy_data and
&
n CREATE SUBSCRIPTION page because if the
user leaves the default (copy_data=true) then the ALTER might cause
some unexpected errors is the subscription was already using binary
format.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
conveniently placed (DEBUG?)
logging.
This way the TAP test can do a 'wait_for_log' instead of the
'poll_query_until'. It will probably generate lots of extra logging
but it still might be lots faster that current code because it won't
incur the 1s overheads of the stats.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Fri, Mar 10, 2023 at 3:32 PM Amit Kapila wrote:
>
> On Thu, Mar 9, 2023 at 10:56 AM Peter Smith wrote:
> >
> > 2. rollback_prepared_cb_wrapper
> >
> > /*
> > * If the plugin support two-phase commits then rollback prepared callback
> > * is ma
explicitly inserting the NULLs?
~~~
9. Unique index that is not primary key or replica identity
9a.
Why are other "Testcase start" comments all uppercase but not this one?
~~~
10. SUBSCRIPTION USES INDEX WITH PUB/SUB different data
Why is there a mixed case in this "Test case:" comment?
~~~
11. PUBLICATION LACKS THE COLUMN ON THE SUBS INDEX
11a.
# The subsriber has an additional column compared to publisher,
# and the index is on that column. We still pick the index scan
# on the subscriber even though it is practically similar to
# sequential scan
Typo "subsriber"
Missing full-stop on last sentence.
~
11b.
# make sure that the subscriber has the correct data
# we only deleted 1 row
$result = $node_subscriber->safe_psql('postgres',
"SELECT sum(x) FROM test_replica_id_full");
is($result, qq(232), 'ensure subscriber has the correct data at the
end of the test');
Why does that say "deleted 1 row", when the previous operation was not a DELETE?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ine isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
~
PSA a small patch that does this.
(Tests OK using make check-world)
Thoughts?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Add-macros-for-ReorderBu
bool did_write,
bool finished_xact)
--
[1]
https://www.postgresql.org/message-id/OS3PR01MB6275C6CA7C0C23730A319EAD9%40OS3PR01MB6275.jpnprd01.prod.outlook.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPt3ZEMo-KTF%3D5KJSU%2BHdWJD19GPGGCKOmBeM47484Ychw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
of the useful (but costly)
subscription tests from the mainstream other tests. Then they won't
cost any extra time for the build-farm, but at least we can still run
them on-demand using PG_TEST_EXTRA [1] approach if we really want to.
--
[1] https://www.postgresql.org/docs/devel/regress-run.html
Kind Regards,
Peter Smith.
Fujitsu Austrlia
tionality even when patch 0002 is not applied yet.
--
[1] Replies to my review v35 -
https://www.postgresql.org/message-id/CACawEhXnTQyGNCXeQGhN3_%2BGWujhS3MyY27C4sSqRvZ%2B_B7FLg%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPvLvDGFzk4fSaevGY5h2PpAeSZjJjje_7vBdb7ag%3DzswA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Mar 8, 2023 at 3:03 PM Amit Kapila wrote:
>
> On Wed, Mar 8, 2023 at 9:09 AM Peter Smith wrote:
> >
> >
> > ==
> > src/backend/executor/execReplication.c
> >
> > 3. build_replindex_scan_key
> >
> > {
> > Oid opera
cal attributes to remote ones */
bool updatable; /* Can apply updates/deletes? */
+ Oid localindexoid; /* which index to use, or InvalidOid if none */
Indentation is not correct for that new member comment.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Mar 6, 2023 at 7:40 PM Amit Kapila wrote:
>
> On Mon, Mar 6, 2023 at 1:40 PM Peter Smith wrote:
> >
...
> >
> > Anyhow, if you feel those firstterm and FULL changes ought to be kept
> > separate from this RI patch, please let me know and I will propose
>
7sF2%3DqzjhuQNkv%2BZvgaTzFv7rs-LA4c2w%40mail.gmail.com
[2]
https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-REPLICA-IDENTITY
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-PGDOCS-replica-identity-quotes.patch
Description: Binary data
On Tue, Mar 7, 2023 at 8:01 PM Amit Kapila wrote:
>
> On Tue, Mar 7, 2023 at 1:19 PM Peter Smith wrote:
> >
> > Let me give an example to demonstrate why I thought something is fishy here:
> >
> > Imagine rel has a (non-default) REPLICA IDENTITY with Oid=.
the RI) --> == --> true;
IdxIsRelationIdentityOrPK(rel, ) --> GetRelationIdentityOrPK(rel)
returns (the valid oid of the RI) --> == --> false;
~
Now two people are telling me this is OK, but I've been staring at it
for too long and I just don't see how it can be. (??)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Mar 6, 2023 at 5:44 PM Amit Kapila wrote:
>
> On Mon, Mar 6, 2023 at 10:12 AM Peter Smith wrote:
> >
> > 4. IdxIsRelationIdentityOrPK
> >
> > +/*
> > + * Given a relation and OID of an index, returns true if the
> > + * index is relat
eturn false
when previously it returned true. Is it deliberate?
==
.../subscription/t/032_subscribe_use_index.pl
5.
+# Testcase start: SUBSCRIPTION USES INDEX WITH PUB/SUB different data
+#
+# The subscriber has duplicate tuples that publisher does not have.
+# When publsher updates/deletes 1 row, subscriber uses indexes and
+# exactly updates/deletes 1 row.
"and exactly updates/deletes 1 row." --> "and updates/deletes exactly 1 row."
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Mar 3, 2023 at 10:04 PM Amit Kapila wrote:
>
> On Fri, Mar 3, 2023 at 4:13 PM Bharath Rupireddy
> wrote:
> >
> > On Thu, Jan 19, 2023 at 8:36 AM Peter Smith wrote:
> > >
> > > On Wed, Jan 18, 2023 at 6:26 PM Bharath Rup
InvalidOid.
+ */
SUGGESTION
Returns the oid of an index that can be used by a subscriber.
Otherwise, returns InvalidOid.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Mar 3, 2023 at 1:27 PM houzj.f...@fujitsu.com
wrote:
>
> On Friday, March 3, 2023 8:18 AM Peter Smith wrote:
...
> > Anyway, I think this exposes another problem. If you still want the patch
> > to pass
> > the 'finshed_xact' parameter separate
532 AEDT [9834] LOG: received immediate shutdown request
2023-03-03 12:45:45.533 AEDT [9834] LOG: database system is shut down
~~
The patches 0001 and 0002 seem to have accidentally blended together
because AFAICT the error is because patch 0001 is testing something
that is not available until 000
601 - 700 of 1458 matches
Mail list logo
