at "
+ "the end of the transaction."),
GUC_NOT_IN_SAMPLE
},
6a. short description
User PoV behaviour should be the same. Instead, maybe say "controls
the internal behavior" or something like that?
~
6b. long description
IMO the long description shouldn’t mention ‘immediate’ mode first as it does.
BEFORE
If set to immediate, on the publisher side, ...
AFTER
On the publisher side, ...
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Smith, Kuroda Hayato, Amit Kapila
"Pater" --> "Peter"
--
Kind Regards,
Peter Smith.
Fujitsu Australia
s to read and apply them at the end of the transaction.
SUGGESTION
On the subscriber, if the streaming option is set to parallel, it
directs the leader apply worker to send changes to the shared memory
queue or to serialize changes and apply them at the end of the
transaction.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Jan 27, 2023 at 10:30 PM Peter Eisentraut
wrote:
>
> On 19.01.23 00:45, Peter Smith wrote:
> > The original $SUBJECT requirements evolved to also try to make each
> > view appear on a separate page after that was suggested by DavidJ [2].
> > I was unable to achieve
t describe
> individual parameters at all, just refer to CREATE PUBLICATION.)
>
The v3 patch LGTM (just for the logical replication commands).
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Jan 31, 2023 at 4:00 AM Tom Lane wrote:
>
> Peter Smith writes:
> > The v3 patch LGTM (just for the logical replication commands).
>
> Pushed then.
>
Thanks for pushing the v3 patch.
I'd forgotten about the 'streaming' option -- AFAIK this was
previ
On Mon, Jan 30, 2023 at 5:23 PM houzj.f...@fujitsu.com
wrote:
>
> On Monday, January 30, 2023 12:13 PM Peter Smith
> wrote:
> >
> > Here are my review comments for v88-0002.
>
> Thanks for your comments.
>
> >
> > ==
> > General
> >
>
Thanks for the updates to address all of my previous review comments.
Patch v90-0001 LGTM.
--
Kind Reagrds,
Peter Smith.
Fujitsu Australia
NGES_THRESHOLD == 0)
rb->update_progress_txn(rb, txn, change->lsn);
--
Kind Regards,
Peter Smith.
Fujitsu Australia
tionInfo
@@ -661,6 +661,7 @@ typedef struct _SubscriptionInfo
char*subdisableonerr;
char*suborigin;
char*subsynccommit;
+ int subminapplydelay;
char*subpublications;
} SubscriptionInfo;
Should this also be "int32" to match the other member type changes?
==
sr
paragraph, and
also same wording as the GUC hint text).
--
Kind Regards,
Peter Smith.
Fujitsu Australia
enclause(Form_pg_trigger trigrec, Node *whenClause,
bool pretty)
+{
It seemed "Parse back" is a typo.
I assume it was meant to say something like "Passes back", or maybe
just "Returns" is better.
==
src/include/replication/logicalrelation.h
12.
@@ -14,6 +14,7 @@
#include "access/attmap.h"
#include "replication/logicalproto.h"
+#include "storage/lockdefs.h"
What is this needed here for? I tried without this change and
everything still builds OK.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
elay) so that
it will *always* log the remaining wait time even if that wait time
becomes negative. Then I think the test cases can be made
deterministic instead of relying on good luck. This seems like the
better option.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Here are my review comments for patch v26-0001.
On Thu, Feb 2, 2023 at 7:18 PM Takamichi Osumi (Fujitsu)
wrote:
>
> Hi,
>
> On Wednesday, February 1, 2023 1:37 PM Peter Smith
> wrote:
> > Here are my review comments for the patch v25-0001.
> Thank you for your rev
On Fri, Feb 3, 2023 at 4:21 PM Amit Kapila wrote:
>
> On Fri, Feb 3, 2023 at 6:41 AM Peter Smith wrote:
> >
> > On Thu, Feb 2, 2023 at 7:21 PM Takamichi Osumi (Fujitsu)
> > wrote:
> > >
> > ...
> > >
> > >
> > > &
On Fri, Feb 3, 2023 at 9:21 PM Alvaro Herrera wrote:
>
> On 2023-Feb-03, Peter Smith wrote:
>
...
> > 3. ExecuteGrantStmt
> >
> > + /* Copy the grantor id needed for DDL deparsing of Grant */
> > + istmt.grantor_uid = grantor;
> > +
> >
> >
char *deparse_ddl_json_to_string(char *jsonb);
+extern char *deparse_drop_command(const char *objidentity, const char
*objecttype,
+ DropBehavior behavior);
+
+
+#endif /* DDL_DEPARSE_H */
Double blank lines.
==
src/include/tcop/deparse_utility.h
18.
@@ -100,6 +103,12 @@ typedef struct CollectedCommand
{
ObjectType objtype;
} defprivs;
+
+ struct
+ {
+ ObjectAddress address;
+ Node *real_create;
+ } ctas;
} d;
All the other sub-structures have comments. IMO this one should have a
comment too.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
q(0), "check the delayed transaction was not applied");
"expectedly" ??
SUGGESTION (for comment)
# Confirm the record was not applied
--
Kind Regards,
Peter Smith.
Fujitsu Australia
anslator considerations here why not write the
first error like:
errmsg("invalid value for parameter \"min_apply_delay\": \"%s\"",
input_string)
~
5b.
Since there are no translator considerations here why not write the
second error like:
errmsg("%d ms is outside the valid range for parameter
\"min_apply_delay\" (%d .. %d)",
result, 0, PG_INT32_MAX))
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ablesync copy phase is completed
* (sublsn NULL) */
-#define SUBREL_STATE_SYNCDONE 's' /* synchronization finished in front of
- * apply (sublsn set) */
+#define SUBREL_STATE_PRE_SYNCDONE 'p' /* synchronization finished in front of
+ * apply (sublsn set), but the final
+ * cleanup has not yet been performed */
+#define SUBREL_STATE_SYNCDONE 's' /* synchronization complete */
#define SUBREL_STATE_READY 'r' /* ready (sublsn set) */
Some adjustments to states and comments are needed according to my
"General Comment".
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Feb 7, 2023 at 4:02 PM Amit Kapila wrote:
>
> On Tue, Feb 7, 2023 at 10:07 AM Kyotaro Horiguchi
> wrote:
> >
> > At Tue, 7 Feb 2023 09:10:01 +0530, Amit Kapila
> > wrote in
> > > On Tue, Feb 7, 2023 at 6:03 AM Peter Smith wrote:
> > >
undant because this was already
explained in the big comment up-front (see #3). Only one useful
sentence is left.
SUGGESTION
Before doing the insertion, get the current timestamp that will be
used as a comparison base.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Tue, Feb 7, 2023 at 6:46 PM houzj.f...@fujitsu.com
wrote:
>
> On Tuesday, February 7, 2023 12:12 PM Peter Smith
> wrote:
> > On Fri, Feb 3, 2023 at 6:58 PM houzj.f...@fujitsu.com
> >
> > wrote:
> > >
> > ...
> > > > Right, I thi
cient, but in practice,
I think the overhead of a single variable assignment every loop
iteration (which is doing WaitLatch anyway) is of insignificant
concern, whereas having one assignment is simpler than having two IMO.
But, if you want to keep it the way you have then that is OK.
Otherwise, this patch v32 LGTM.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
And
> add a new state PRE_SYNCDONE after synchronization finished in front of apply
> (sublsn set), but before dropping the origin and other final cleanups. The
> tablesync will restart and redo the cleanup if it failed after reaching the
> new
> state. Besides, since the changes can already be applied on the table in
> PRE_SYNCDONE state, so I also modified the check in
> should_apply_changes_for_rel(). And some other conditions for the origin drop
> in subscription commands are were adjusted in this patch.
>
BTW, the tablesync.c has a large file header comment which describes
all about the relstates including some examples. So this patch needs
to include modifications to that comment.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
Hi Vignesh, thanks for addressing my v63-0002 review comments.
I confirmed most of the changes. Below is a quick follow-up for the
remaining ones.
On Mon, Feb 6, 2023 at 10:32 PM vignesh C wrote:
>
> On Mon, 6 Feb 2023 at 06:47, Peter Smith wrote:
> >
...
> >
her xmlFreeDoc(doc) is not. As the doc is assigned outside the
PG_TRY shouldn't those both be the same?
------
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Thu, Feb 9, 2023 at 10:42 AM Jim Jones wrote:
>
> On 09.02.23 00:09, Peter Smith wrote:
> > I noticed the xmlFreeDoc(doc) within the PG_CATCH is guarded but the
> > other xmlFreeDoc(doc) is not. As the doc is assigned outside the
> > PG_TRY shouldn't those b
> The comment adjustment suggested by Peter-san above
> was also included in this v33.
> Please have a look at the attached patch.
Patch v33 LGTM.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
=> 'xml', prosrc => 'xmlpretty' },
Spurious leading space for this new entry.
==
doc/src/sgml/func.sgml
5.
+
+
+ 42
+
+
+
+(1 row)
+
+]]>
A spurious blank line in the example after the "(1 row)"
~~~
6.
Does this function docs belong in section 9.15.1 "Producing XML
Content"? Or (since it is not really producing content) should it be
moved to the 9.15.3 "Processing XML" section?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
er is doing
the sending, but IIUC it's really the opposite.
And since WAIT_EVENT_LOGICAL_PARALLEL_APPLY_LEADER_SEND_DATA seems too
verbose, how about shortening the prefix for both events? E.g.
BEFORE
WAIT_EVENT_LOGICAL_PARALLEL_APPLY_SEND_DATA,
WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE,
AFTER
WAIT_EVENT_LOGICAL_PA_LEADER_SEND_DATA,
WAIT_EVENT_LOGICAL_PA_STATE_CHANGE,
--
Kind Regards,
Peter Smith.
Fujitsu Australia
"SELECT pid != $oldpid FROM pg_stat_replication WHERE
application_name = 'tap_sub_renamed' AND state = 'streaming';"
+ )
+ or die
+ "Timed out while waiting for the walsender to restart after
changing min_apply_delay to non-zero value";
IIUC this test is for verifying that a new walsender worker was
started if the delay was changed from 0 to non-zero. E.g. I think it
is for it is testing your new logic of the maybe_reread_subscription.
Probably more complete testing also needs to check the other scenarios:
* min_apply_delay from one non-zero value to another non-zero value
--> verify a new worker is NOT started.
* change min_apply_delay from non-zero to zero --> verify a new worker
IS started
--
Kind Regards,
Peter Smith.
Fujitsu Australia
mat: invalid string (whitespaces)
--
[1]
https://api.cirrus-ci.com/v1/artifact/task/4802219812323328/testrun/build/testrun/regress/regress/regression.diffs
Kind Regards,
Peter Smith.
Fujitsu Australia
not.
==
src/include/replication/walreceiver.h
6.
Should the protocol version be bumped (and documented) now that the
START REPLICATION supports a new extended syntax? Or is that done only
for messages sent by pgoutput?
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
option #4 result might look like this:
test_sub=# create subscription sub1 connection 'dbname=test_pub'
publication pub1;
NOTICE: created replication slot "sub1" on publisher CREATE SUBSCRIPTION
NOTICE: subscription "sub1" skipping DDL: create subscription
sub_node2 connection 'dbname=postgres host=node1 port=5432'
publication pub_node1;
...
(And if it turns out users really do want this then it can be
revisited in later patch versions)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
CUUM_TRUNCATE
+ WAIT_EVENT_VACUUM_TRUNCATE,
+ WAIT_EVENT_LOGICAL_APPLY_DELAY
} WaitEventTimeout;
FYI - The PGDOCS has a section with "Table 28.13. Wait Events of Type
Timeout" so if you a going to add a new Timeout Event then you also
need to document it (alphabetically) in that table.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
that looked pre-built for 'coverage'. Did I miss
something, or is it a case of just invent-your-own extra tests/values
for your own ad-hoc requirements?
e.g.
make check EXTRA_TESTS=extra_regress_for_coverage
make check-world PG_TEST_EXTRA='extra_tap_tests_for_coverage'
Thanks!
--
[1] https://www.postgresql.org/docs/devel/regress-run.html
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Feb 14, 2023 at 10:44 AM Andres Freund wrote:
>
> Hi,
>
> On 2023-02-14 09:26:47 +1100, Peter Smith wrote:
> > I've observed suggested test cases get rejected as being overkill, or
> > because they would add precious seconds to the test execution. OTOH, I
&g
FYI - the latest patch cannot be applied.
See cfbot [1]
--
[1] http://cfbot.cputube.org/patch_42_3595.log
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Feb 14, 2023 at 5:04 PM Amit Kapila wrote:
>
> On Fri, Feb 10, 2023 at 8:56 AM Peter Smith wrote:
> >
> > On Fri, Feb 10, 2023 at 1:32 PM houzj.f...@fujitsu.com
> > wrote:
> > >
> > > On Tuesday, February 7, 2023 11:17 AM Amit Kapila
> >
l,
but the other XML tests are working OK again.
Those results implied to me that this function code (in my environment
anyway) is somehow introducing a side effect causing the *other* XML
tests to fail.
But so far I was unable to identify the reason. Sorry, I don't know
this XML API well enough to help more.
--
Kind Regards,
Peter Smith.
Fujitsu Austalia.
On Wed, Feb 15, 2023 at 11:05 AM Jim Jones wrote:
>
> On 14.02.23 23:45, Peter Smith wrote:
> > Those results implied to me that this function code (in my environment
> > anyway) is somehow introducing a side effect causing the *other* XML
> > tests to fail.
>
> I be
ild partition of
+ * edata->targetRelInfo, find the index on the partition.
+ *
+ * Note that if the corresponding relmapentry has invalid usableIndexOid,
+ * the function returns InvalidOid.
+ */
"(e.g., the relation)" --> "(i.e. the relation)"
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sat, Feb 11, 2023 at 3:21 AM vignesh C wrote:
>
> On Thu, 9 Feb 2023 at 03:47, Peter Smith wrote:
> >
> > Hi Vignesh, thanks for addressing my v63-0002 review comments.
> >
> > I confirmed most of the changes. Below is a quick follow-up for the
> > remaini
On Thu, Feb 9, 2023 at 8:55 PM Ajin Cherian wrote:
>
> On Fri, Feb 3, 2023 at 11:41 AM Peter Smith wrote:
> >
> > Here are some review comments for patch v63-0001.
> >
> > ==
> > src/backend/catalog/aclchk.c
> >
> > 3. ExecuteGrantStmt
On Wed, Feb 15, 2023 at 6:10 PM Jim Jones wrote:
>
> On 15.02.23 02:09, Peter Smith wrote:
> > With v8, in my environment, in psql I see something slightly different:
> >
> >
> > test_pub=# SET XML OPTION CONTENT;
> > SET
> > test_pub=# SELECT xmlfo
XML format: empty string
SELECT xmlformat('');
ERROR: invalid XML document
-\set VERBOSITY default
\ No newline at end of file
+\set VERBOSITY default
--
The attached patch update (v12-0002) fixes the xml_2.out for me.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v12-0001-A
sending data from the apply
worker, but we should have invented a new wait state since the code was
waiting at a new place.
This patch corrects the mistake by using a new wait state
"LogicalApplySendData".
--
Kind Regards,
Peter Smith.
Fujitsu Australia
w test has exposed. Maybe I am mistaken
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Feb 17, 2023 at 5:57 PM Peter Smith wrote:
>
> FYI, I accidentally left this (v23) patch's TAP test
> t/032_subscribe_use_index.pl still lurking even after removing all
> other parts of this patch.
>
> In this scenario, the t/032 test gets stuck (build of latest H
_POLL) && defined(POLLRDHUP))
+ return true;
+#else
+ return false;
+#endif
+}
~
8a.
"can work well" --> "works"
~
8b.
Maybe better to say "true (1)" and "otherwise false (0)"
--
Kind Regards,
Peter Smith.
Fujitsu Australia
-64,6 +68,7 @@ typedef struct LogicalDecodingContext
LogicalOutputPluginWriterPrepareWrite prepare_write;
LogicalOutputPluginWriterWrite write;
LogicalOutputPluginWriterUpdateProgress update_progress;
+ LogicalOutputPluginWriterDelay delay;
~
15a.
Question: Is there some advantage to introducing another callback,
instead of just doing the delay inline?
~
15b.
Should this be a more informative member name like 'delay_send'?
~~~
16.
@@ -100,6 +105,8 @@ typedef struct LogicalDecodingContext
*/
bool twophase_opt_given;
+ int32 min_send_delay;
+
Missing comment for this new member.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
d apply delay.
+ok( time() - $publisher_insert_time >= $delay,
+ "subscriber applies WAL only after replication delay for
non-streaming transaction"
+);
It's not strictly an "apply delay". Maybe this comment only needs to
say like below:
SUGGESTION
# This test is successful only if at least the configured delay has elapsed.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
d timeout is disabled,
+ *
+ * - If the timeout is disabled, try to check the health of the socket
+ * - Otherwise this immediately returns 0
+ *
+ * Return >0 if condition is met, 0 if a timeout occurred, -1 if an error
+ * or interrupt occurred.
Don't say "and timeout is disabled,"
d.
--
[1] Kuroda-san replied to my review v3-0001.
https://www.postgresql.org/message-id/TYAPR01MB5866C6BCA4D9386D9C486033F5A59%40TYAPR01MB5866.jpnprd01.prod.outlook.com
[2] My previous review v3-0001.
https://www.postgresql.org/message-id/CAHut%2BPu6Y%2BBkYKg6MYGi2zGnx6imeK4QzxBVhpQoPMeDr1npnQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
nothing else to do...e.g. no need for a new extra static
boolean if static RelationSyncCache is acting as the one-time guard
anyway.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Sep 21, 2022 at 8:22 PM Peter Smith wrote:
>
...
> > > On Wed, Sep 21, 2022 at 3:23 PM Amit Kapila
> > > wrote:
> > > ...
> > >
> > > > Can't we use the existing function ReplicationOriginNameForTablesync()
> > > &
hat
would be useful (my patch does not do this).
~~
In passing, I also made a 0002 patch to remove some inconsistent
whitespace noticed in those config tables.
Thoughts?
------
Kind Regards,
Peter Smith.
Fujitsu Australia.
v1-0001-GUC-tables-used-designated-initializers.patch
Description: Bi
int max_logical_replication_workers = 0;
int max_sync_workers_per_subscription = 0;
Thoughts?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Sep 27, 2022 at 10:08 AM Tom Lane wrote:
>
> Peter Smith writes:
> > It seems confusing to me that for the above code the initial value is
> > "hardwired" in multiple places. Specifically, it looks tempting to
> > just change the variable declarati
for the size I quickly found:
File: src/bin/pg_dump/pg_dump_sort.c:
static void
describeDumpableObject(DumpableObject *obj, char *buf, int bufsize)
caller:
describeDumpableObject(loop[i], buf, sizeof(buf));
~~
I expect you can find more like just this if you look harder than I did.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
blications. The partition table whose ancestor is
+ * also published in this publication array will be filtered out in this
+ * function.
+ */
--
[1]
https://www.postgresql.org/message-id/OS0PR01MB5716A30DDEECC59132E1084F94799%40OS0PR01MB5716.jpnprd01.prod.outlook.com
[2] https://www.postgresql.org/docs/devel/sql-createpublication.html
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Sep 27, 2022 at 11:07 AM Peter Smith wrote:
>
...
> I will try to post a patch in the new few days to address (per your
> suggestions) some of the variables that I am more familiar with.
>
PSA a small patch to tidy a few of the GUC C variables - adding
comments and removing
On Wed, Sep 28, 2022 at 2:21 AM Tom Lane wrote:
>
> Peter Smith writes:
> > Enums index a number of the GUC tables. This all relies on the
> > elements being carefully arranged to be in the same order as those
> > enums. There are comments to say what enum index belongs t
pg_database_owner
pg_read_all_stats pg_write_server_files TEMPORARY
2a.
grant "GRANT" ??
~
2b.
grant "TEMPORARY" but not "TEMP" ??
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
inline functions anyhow so it should end up as
the same thing, right?
static inline bool
am_leader_apply_worker(void)
{
return (!am_tablesync_worker() && !am_parallel_apply_worker);
}
==
58.
--- fail - streaming must be boolean
+-- fail - streaming must be boolean or 'parallel'
CREATE SUBSCRIPTION regress_testsub CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect =
false, streaming = foo);
I think there are tests already for explicitly create/set the
subscription parameter streaming = on/off/parallel
But what about when there is no value explicitly specified? Shouldn't
there also be tests like below to check that *implied* boolean true
still works for this enum?
CREATE SUBSCRIPTION ... WITH (streaming)
ALTER SUBSCRIPTION ... SET (streaming)
--
[1] My patch snprintfs -
https://www.postgresql.org/message-id/flat/CAHut%2BPsB9hEEU-JHqTUBL3bv--vesUvThYr1-95ZyG5PkF9PQQ%40mail.gmail.com#17abe65e826f48d3d5a1cf5b83ce5271
[2] My patch GUC C vars -
https://www.postgresql.org/message-id/flat/CAHut%2BPsWxJgmrAvPsw9smFVAvAoyWstO7ttAkAq8NKDhsVNa3Q%40mail.gmail.com#1526a180383a3374ae4d701f25799926
[3] Houz reply comment #41 -
https://www.postgresql.org/message-id/OS0PR01MB5716E7E5798625AE9437CD6F94439%40OS0PR01MB5716.jpnprd01.prod.outlook.com
[4] Previous review comment #13 -
https://www.postgresql.org/message-id/CAHut%2BPuVjRgGr4saN7qwq0oB8DANHVR7UfDiciB1Q3cYN54F6A%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Sep 28, 2022 at 12:04 PM Peter Smith wrote:
>
> On Wed, Sep 28, 2022 at 2:21 AM Tom Lane wrote:
> >
> > Peter Smith writes:
> > > Enums index a number of the GUC tables. This all relies on the
> > > elements being carefully arranged to be in the sa
On Tue, Oct 4, 2022 at 5:48 PM Michael Paquier wrote:
>
> On Tue, Oct 04, 2022 at 04:20:36PM +1100, Peter Smith wrote:
> > The v2 patches are updated as follows:
> >
> > 0001 - Now this patch only fixes a comment that had a wrong enum name.
>
> This was wrong,
On Thu, Sep 29, 2022 at 12:50 PM shiy.f...@fujitsu.com
wrote:
>
> On Wed, Sep 28, 2022 1:49 PM Kyotaro Horiguchi
> wrote:
> >
> > At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith
> > wrote in
...
> > >
> > > 2. tab complete for
ONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4
:constbyval true :constisnull false
:location 53 :constvalue 4 [ 33 0 0 0 0 0 0 0 ]}) :location 51}")
(2 rows)
--
[1]
https://www.postgresql.org/message-id/OS3PR01MB6275A9B8C65C381C6828DF9D9E549%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
2BPvugkna7avUQLydg602hymc8qMp%3DCRT2ZCTGbi8Bkfv%2BA%40mail.gmail.com
[2] Euler's reply to my v4 review -
https://www.postgresql.org/message-id/acfc1946-a73e-4e9d-86b3-b19cba225a41%40www.fastmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
a parsetree that modified it, return an ObjTree
+ * representing the alter type.
but sometimes - like deparse_AlterExtensionStmt() etc. - they don't
bother to say anything at all.
e.g.
+/*
+ * Deparse ALTER EXTENSION .. UPDATE TO VERSION
+ */
+static ObjTree *
+deparse_AlterExtensionStmt(Oid objectId, Node *parsetree)
Either it is useful, so say it always, or it is not useful, so say it
never. Pick one.
--
6. GENERAL - json
IMO change "json" -> "JSON" everywhere.
Here are some examples:
Line 7605: + * Conversion specifier, it determines how we expand the
json element into
Line 7699: + errmsg("missing element \"%s\" in json object", keyname)));
Line 7857: + * Expand a json value as a quoted identifier. The value
must be of type string.
Line 7872: + * Expand a json value as a dot-separated-name. The value
must be of type
Line 7908: + * Expand a json value as a type name.
Line 7966: + * Expand a json value as an operator name
Line 7993: + * Expand a json value as a string. The value must be of
type string or of
Line 8031: + * Expand a json value as a string literal.
Line 8070: + * Expand a json value as an integer quantity.
Line 8083: + * Expand a json value as a role name. If the is_public
element is set to
Line 8111: + * Expand one json element into the output StringInfo
according to the
Line 8807: +{ oid => '4642', descr => 'deparse the DDL command into
json format string',
Line 8810: +{ oid => '4643', descr => 'expand json format DDL to a
plain DDL command',
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ub' publication pub1 with (connect = false);
WARNING: tables were not subscribed
HINT: You will have to run ALTER SUBSCRIPTION ... REFRESH PUBLICATION
to subscribe the tables.
CREATE SUBSCRIPTION
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-create-subscription-warning-message.patch
Description: Binary data
On Thu, Oct 6, 2022 at 10:38 PM Amit Kapila wrote:
>
> On Fri, Sep 30, 2022 at 1:56 PM Peter Smith wrote:
> >
> > Here are my review comments for the v35-0001 patch:
> >
> > ==
> >
> > 3. GENERAL
> >
> > (this comment was written after
On Sat, Oct 8, 2022 at 2:23 AM Tom Lane wrote:
>
> Peter Smith writes:
> > WARNING: tables were not subscribed, you will have to run ALTER
> > SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables
>
> > When I first encountered the above CREATE SUBSCRIPTION wa
(e.g. for making an
empty "test" database)? Will just saying publish='ddl' allow that?
- etc.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
gives accurate information:
> WARNING: subscription was created, but is not connected
> HINT: You should create the slot manually, enable the subscription,
> and run %s to initiate replication.
>
+1
PSA patch v2 worded like that.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Oct 11, 2022 at 2:46 PM Amit Kapila wrote:
>
> On Mon, Oct 10, 2022 at 8:14 PM Tom Lane wrote:
> >
> > Peter Smith writes:
> > > On Mon, Oct 10, 2022 at 4:04 PM Amit Kapila
> > > wrote:
> > >> I think the below gives accurate informatio
not really a boolean
option anymore - it's a kind of a hybrid boolean/enum. That's why I
thought this ought to be tested regardless if there are existing tests
for the (normal) boolean options.
Anyway, you can decide what you want.
--
[1] Houzj replies to my v35 review
https://www.
t; >> separate patch. What do you think?
>
> > No objection.
>
> Yeah, the v3-0001 patch is fine. I agree that the docs change needs
> more work.
Thanks to everybody for the feedback/suggestions. I will work on
improving the documentation for this and post something i
y be published as either INSERT or UPDATE, or it may
not be published at all.
SUGGESTION
For an INSERT ... ON CONFLICT command, the publication will publish
the operation that results from the command. Depending on the outcome,
it may be published as either INSERT or UPDATE, or it may not be
only because it
didn't seem to be the typical convention for all the other numeric
GUCs I looked at, but it's fine by me if that way is preferred
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Oct 13, 2022 at 9:07 AM Peter Smith wrote:
>
> On Thu, Oct 13, 2022 at 2:01 AM Tom Lane wrote:
> >
> > Alvaro Herrera writes:
> > > On 2022-Oct-12, Amit Kapila wrote:
> > >> Okay, then I think we can commit the last error message patch of Peter
&g
his implied boolean-true behaviour is
already described elsewhere? But if it is, I have not found it yet.
IMO a simple patch (PSA) is needed to clarify the behaviour.
Thoughts?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-clarify-behavior-of-specifying-a-parameter-with-n.patch
Description: Binary data
On Sun, Oct 16, 2022 at 12:14 AM Amit Kapila wrote:
>
> On Fri, Oct 14, 2022 at 8:22 AM Peter Smith wrote:
> >
> > On Thu, Oct 13, 2022 at 9:07 AM Peter Smith wrote:
> > >
...
> > PSA a patch for adding examples of how to activate a subscription that
> > w
ION ... ADD PUBLICATION" : ALTER
> SUBSCRIPTION ... DROP PUBLICATION")
>
> I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here; sounds
> like ERRCODE_FEATURE_NOT_SUPPORTED might be more appropriate.
>
I thought maybe ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE, which would
make it the same as similar messages in the same function when
incompatible parameters are specified.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
BOyQ8-psruZZ0sYff5KactTHZneR-cfsHd%2Bn%2BN7khEKQ%40mail.gmail.com
[3] Hou's feedback for my v36 review -
https://www.postgresql.org/message-id/OS0PR01MB57162232BF51A09F4BD13C7594249%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Oct 11, 2022 at 1:28 AM shiy.f...@fujitsu.com
wrote:
>
> On Mon, Oct 10, 2022 2:12 PM shiy.f...@fujitsu.com
> wrote:
> >
> > On Tue, Oct 4, 2022 4:17 PM Peter Smith wrote:
> > >
> > > But, while testing I noticed another different quirk
> >
Thanks for the feedback.
On Mon, Oct 17, 2022 at 10:14 PM Amit Kapila wrote:
>
> On Mon, Oct 17, 2022 at 7:17 AM Peter Smith wrote:
> >
> >
> > Updated as sugggested.
> >
>
> +
> +Sometimes, either by choice (e.g. create_slot =
> false),
&g
d example
will then need additional bullets/notes to say – “if there is no
slot_name do this…” and “if there is a slot_name do that…”. It’s
really only the activation part which is identical for both.
-
Kind Regards,
Peter Smith.
Fujitsu Australia
PSA trivial patch to fix a code comment typo seen during a recent review.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-typo-instead-just.patch
Description: Binary data
On Wed, Oct 19, 2022 at 12:29 PM Michael Paquier wrote:
>
> On Wed, Oct 19, 2022 at 10:09:12AM +1100, Peter Smith wrote:
> > PSA trivial patch to fix a code comment typo seen during a recent review.
>
> Passing by.. And fixed. Thanks!
> --
Thanks for passing by.
-
On Tue, Oct 18, 2022 at 7:09 AM Justin Pryzby wrote:
>
> On Fri, Oct 14, 2022 at 07:54:37PM +1100, Peter Smith wrote:
> > Hi hackers.
> >
> > This post is about parameter default values. Specifically. it's about
> > the CREATE PUBLICATION and CREATE SUBSCRI
when creating
> subscription because "connect=false" is not specified.
>
Oh, thanks for finding and reporting that. Sorry for my cut/paste
errors. Fixed in v7. PSA,
> I have tested the examples in the patch and didn't see any problem other than
> the one above.
>
T
have C initial values -
https://www.postgresql.org/message-id/Y0dgHfEGvvay5nle%40paquier.xyz
[2] sanity-check idea -
https://www.postgresql.org/message-id/1113448.1665717297%40sss.pgh.pa.us
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0001-Tidied-some-GUC-C-variable-declarations.patch
Description
On Thu, Oct 20, 2022 at 3:16 PM Justin Pryzby wrote:
>
> On Thu, Oct 20, 2022 at 11:56:58AM +1100, Peter Smith wrote:
> > Patch 0002 adds a sanity-check function called by
> > InitializeGUCOptions, as suggested by Tom [2]. This is to ensure that
> > the GUC C variable ini
l_apply_find_worker() looking for
something we know will not be found?
--
[1] My review of v38-0001 -
https://www.postgresql.org/message-id/CAHut%2BPsY0aevdVqeCUJOrRQMrwpg5Wz3-Mo%2BbU%3DmCxW2%2B9EBTg%40mail.gmail.com
[2] Houz reply for my review v38 -
https://www.postgresql.org/message-id/OS0PR01MB5716D738A8F27968806957B194289%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
fail - http://cfbot.cputube.org/patch_40_3736.log
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Oct 17, 2022 at 4:49 PM wangw.f...@fujitsu.com
wrote:
>
> On Wed, Oct 5, 2022 at 11:08 AM Peter Smith wrote:
> > Hi Wang-san. Here are my review comments for HEAD_v12-0001 patch.
>
...
> >
> > 3. QUESTION - pg_get_publication_tables / fetch_table_list
>
at about some description to explain the second example?
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPt%2B1PNx6VsZ-xKzAU-18HmNXhjCC1TGakKX46Wg7YNT1Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
901 - 1000 of 1980 matches
Mail list logo
