On Tue, Jun 13, 2023 at 2:25 PM Amit Kapila wrote:
>
> On Fri, May 12, 2023 at 3:28 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> >
> > I can reproduce this via gdb following similar steps in [1].
> >
> > I think we need to move this call into a transaction as wel
01 and
0008 and avoid having an additional ObjTree layer unless you or others
feel we need it. I think that will reduce the overall patch size and
help us to focus on one of the approaches. Surely, as suggested by
you, we should also evaluate if we can generate this code for the
various command types.
--
With Regards,
Amit Kapila.
On Tue, Jun 13, 2023 at 2:25 PM Amit Kapila wrote:
>
> On Fri, May 12, 2023 at 3:28 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> >
> > I can reproduce this via gdb following similar steps in [1].
> >
> > I think we need to move this call into a transaction as wel
ssue following the steps mentioned by you
and the proposed patch to fix the issue looks good to me.
--
With Regards,
Amit Kapila.
On Thu, Jun 8, 2023 at 5:32 PM shveta malik wrote:
>
> On Thu, Jun 8, 2023 at 10:36 AM Amit Kapila wrote:
>
> Please find new set of patches addressing below:
> a) Issue mentioned by Wang-san in [1],
> b) Comments from Peter given in [2]
> c) Comments from Amit given in the
ning->oldestRunningXid ==
running->nextXid)". I think the main problem is that we started
decoding immediately from the point where we restored a snapshot as at
that point we could have some partial running xacts.
--
With Regards,
Amit Kapila.
don't do that for other GUC-depends propety.
>
Yeah, that would probably be a good idea but do we want to do it in
default mode? I think if we want to optimize the default mode such
that we only WAL log the DDL with user-specified options then
appending options for default GUCs would make the string to be WAL
logged unnecessarily long. I am thinking that in default mode we can
allow subscribers to perform operations with their default respective
GUCs.
--
With Regards,
Amit Kapila.
On Thu, Jun 8, 2023 at 6:32 AM Masahiko Sawada wrote:
>
> On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila wrote:
> >
> > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada
> > wrote:
> > >
> > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila
> > >
On Tue, Jun 6, 2023 at 4:26 PM Amit Kapila wrote:
>
>
> Few assorted comments:
> ===
>
Few comments on 0008* patch:
==
1. After 0008*, deparse_CreateStmt(), forms dpcontext before forming
identity whereas it is required only after it. It
On Tue, Jun 6, 2023 at 7:45 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Monday, June 5, 2023 8:03 AM Amit Kapila wrote:
> >
> >
> > I am able to reproduce the problem with the script and steps mentioned by
> > you. The patch provided by you fixes the problem and looks goo
>
This part needs some analysis/thoughts. BTW, do you mean that it skips
the assignment (a) because the assignment record is before we reach a
consistent point, or (b) because we start reading WAL after the
assignment, or (c) something else? If you intend to say (a) then can
you please point me to the code you are referring to for the same?
--
With Regards,
Amit Kapila.
b/src/test/modules/test_ddl_deparse_regress/t/001_compare_dumped_results.pl.orig
Will this file require for the 0008 patch? Or is this just a leftover?
--
With Regards,
Amit Kapila.
to do this refactoring for v16.
However, sometimes, we do decide to backpatch refactoring in tests to
avoid backpatch effort. I am not completely sure if that is the case
here.
--
With Regards,
Amit Kapila.
On Wed, May 17, 2023 at 8:34 AM Amit Kapila wrote:
>
> On Wed, May 17, 2023 at 7:18 AM Zhijie Hou (Fujitsu)
> wrote:
> >
> >
> > The attached tap test shows how the failure happened.
> >
>
> I haven't yet tried to reproduce it but will try later som
On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada wrote:
>
> On Thu, May 25, 2023 at 5:41 PM Amit Kapila wrote:
>
> I've attached the updated patch. Please review it.
>
Few comments:
1.
+ /* get the owner for ACL and RLS checks */
+ run_as_owner = MySubscription->r
ALL", see [2])
>
Right, this should be changed.
--
With Regards,
Amit Kapila.
On Thu, May 25, 2023 at 12:33 PM Masahiko Sawada wrote:
>
> On Tue, May 23, 2023 at 8:21 PM Amit Kapila wrote:
> >
> > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada
> > wrote:
> > >
> > > Thank you for updating the patch! Here are review comments:
&g
> by a user who doesn't have the permission to the target table. The
> test passes only if run_as_owner = true works.
>
Why in the test do you need to give additional permissions to
regress_admin2 when the actual operation has to be performed by the
table owner?
+# Because the initial data
an
> specify what objects to sync. But it would make the feature complex
> and I'm not sure users can use it properly.
>
> Third idea is that since the use case of synchronizing the whole
> database can be achievable even by pg_dump(all), we support
> synchronizing only tab
ared here but without the patch or some further
discussion, it won't be easy to conclude that.
Tom/Nathan, do you have any further suggestions here?
[1] -
https://www.postgresql.org/message-id/capphfdsld6e--epngqxenqlp6dlwunzrpmcnyb3wj87wr7u...@mail.gmail.com
--
With Regards,
Amit Kapila.
e BF failure[2] (it's very rare and only happened once in 4
> months) which I think is due to the low frequent reload in apply worker.
>
> The attached tap test shows how the failure happened.
>
I haven't yet tried to reproduce it but will try later sometime.
Thanks for your analysis.
[1] - https://www.postgresql.org/message-id/2138662.1623460441%40sss.pgh.pa.us
--
With Regards,
Amit Kapila.
eration on a particular table, it will switch roles to the table
owner and perform the operation with the table owner's privileges." to
be bit more specific about initial sync process as well?
[1] - https://www.postgresql.org/docs/devel/logical-replication-security.html
--
With Regards,
Amit Kapila.
On Fri, May 12, 2023 at 5:25 PM Ajin Cherian wrote:
>
> On Fri, May 12, 2023 at 1:49 PM Amit Kapila wrote:
> >
>
> I tried the following test:
>
>
> Repeat On the publisher and subscriber:
> /* Create role regress_alice with NOSUPERUSER on
>
On Fri, May 12, 2023 at 10:33 AM Amit Kapila wrote:
>
> On Fri, May 12, 2023 at 7:38 AM Masahiko Sawada wrote:
> >
> > On Thu, May 11, 2023 at 2:04 PM Amit Kapila wrote:
> > >
> > > On Fri, Apr 28, 2023 at 2:35 PM Hayato Kuroda (Fujitsu)
> &
On Fri, May 12, 2023 at 7:38 AM Masahiko Sawada wrote:
>
> On Thu, May 11, 2023 at 2:04 PM Amit Kapila wrote:
> >
> > On Fri, Apr 28, 2023 at 2:35 PM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Dear hackers,
> > >
> > &
On Fri, May 12, 2023 at 9:10 AM Masahiko Sawada wrote:
>
> On Fri, May 12, 2023 at 1:12 AM Robert Haas wrote:
> >
> > On Thu, May 11, 2023 at 7:38 AM Amit Kapila wrote:
> > > Do we want the initial sync to also respect 'run_as_owner' option? I
> > &
n? I
might be missing something but I don't see anything in the docs about
initial sync interaction with this option. In the commit a2ab9c06ea,
we did the permission checking during the initial sync so I thought we
should do it here as well.
--
With Regards,
Amit Kapila.
pe56T2D%3DeuO-Qs-GAg%40mail.gmail.com
--
With Regards,
Amit Kapila.
roposed patch at that time but Kuroda-San came up
with a new patch with a different design based on the discussion here.
I haven't looked at it yet though.
[1] -
https://about.gitlab.com/blog/2019/02/13/delayed-replication-for-disaster-recovery-with-postgresql/
[2] - https://dev.mysql.com/doc/refman/8.0/en/replication-delayed.html
[3] -
https://www.postgresql.org/message-id/73b06a32-56ab-4056-86ff-e307f3c316f1%40www.fastmail.com
--
With Regards,
Amit Kapila.
On Tue, May 9, 2023 at 12:44 PM Drouvot, Bertrand
wrote:
>
> On 5/9/23 8:02 AM, Amit Kapila wrote:
> > On Mon, May 8, 2023 at 1:45 PM Drouvot, Bertrand
> > wrote:
>
> >
> > Why not initialize the cascading standby node just before the standby
> > prom
WalSndWakeup(switchedTLI, true);
> + ConditionVariableBroadcast(&WalSndCtl->cv);
>
> After the change, we wakeup physical walsender regardless of switchedTLI flag.
> Is this intentional ? if so, I think It would be better to update the
> comments above this.
>
This raises the question of whether we need this condition variable
logic for physical walsenders?
--
With Regards,
Amit Kapila.
ate. It is not clear why this could happen because
apply worker should eventually update these relations state to 'r'. If
this is reproducible, we can try two things to further investigate the
issue: (a) Disable and Enable the subscription once and see if that
helps; (b) try increasing the LOG level to DEBUG2 and see if we get
any useful LOG message.
--
With Regards,
Amit Kapila.
On Tue, May 9, 2023 at 7:50 AM Masahiko Sawada wrote:
>
> On Mon, May 8, 2023 at 8:09 PM Amit Kapila wrote:
> >
> >
> > I think it is only possible for the leader apply can worker to try to
> > receive the error message from an error queue after your 0002 patch.
>
etting slot_name=NONE. It would be better to
discuss this in a separate thread to seek the opinion of others.
--
With Regards,
Amit Kapila.
On Mon, May 8, 2023 at 1:45 PM Drouvot, Bertrand
wrote:
>
> On 5/8/23 4:42 AM, Amit Kapila wrote:
> > On Sat, May 6, 2023 at 9:33 PM Drouvot, Bertrand
> > wrote:
> >>
> >> On 5/6/23 3:28 PM, Amit Kapila wrote:
> >>> On Sat, May 6, 2023 at 1:52 PM
On Fri, May 5, 2023 at 9:14 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, May 3, 2023 3:17 PM Amit Kapila wrote:
> >
>
> Attach another patch to fix the problem that pa_shutdown will access invalid
> MyLogicalRepWorker. I personally want to avoid introducing new stati
On Mon, May 8, 2023 at 11:08 AM Masahiko Sawada wrote:
>
> On Mon, May 8, 2023 at 12:52 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Monday, May 8, 2023 11:08 AM Masahiko Sawada
> >
> > Hi,
> >
> > >
> > > On Tue, May 2, 2023 at 12:22 PM
NNECTION_FAILURE),
errmsg("could not connect to publisher when attempting to drop
replication slot \"%s\": %s",
slotname, err),
/* translator: %s is an SQL ALTER command */
errhint("Use %s to disassociate the subscription from the slot.",
"ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
...
}
--
With Regards,
Amit Kapila.
On Sat, May 6, 2023 at 9:33 PM Drouvot, Bertrand
wrote:
>
> On 5/6/23 3:28 PM, Amit Kapila wrote:
> > On Sat, May 6, 2023 at 1:52 PM Drouvot, Bertrand
> > wrote:
>
> > Next steps:
> > =
> > 1. The first thing is we should verify this theory by add
by initializing a
fresh node for primary and standby where we only have logical slots on
standby. This will be a bit costly but probably less risky. (c) any
better ideas?
--
With Regards,
Amit Kapila.
On Fri, May 5, 2023 at 7:53 PM Drouvot, Bertrand
wrote:
>
>
> On 5/5/23 2:28 PM, Amit Kapila wrote:
> > On Fri, May 5, 2023 at 5:36 PM Drouvot, Bertrand
> > wrote:
> >
> > It seems due to some reason the current wal file is not switched due
> > to some rea
On Fri, May 5, 2023 at 5:36 PM Drouvot, Bertrand
wrote:
>
> On 5/5/23 12:58 PM, Amit Kapila wrote:
> > On Fri, May 5, 2023 at 4:02 PM Drouvot, Bertrand
> > wrote:
>
> > How did you concluded that 00010003 is the file the
> > test is expecting to
On Fri, May 5, 2023 at 4:02 PM Drouvot, Bertrand
wrote:
>
> On 5/5/23 11:29 AM, Amit Kapila wrote:
> > On Fri, May 5, 2023 at 1:16 PM Drouvot, Bertrand
> > wrote:
> >>
> >>
> >> After multiple attempts, I got one failing one.
> >>
e? Is it possible to share some minimal test case to show the
exact problem you are facing?
[1] -
https://www.postgresql.org/message-id/20230217075433.u5mjly4d5cr4hcfe%40jrouhaud
[2] -
https://www.postgresql.org/message-id/TYAPR01MB58664C81887B3AF2EB6B16E3F5939%40TYAPR01MB5866.jpnprd01.prod.outlook.com
--
With Regards,
Amit Kapila.
On Fri, May 5, 2023 at 2:59 PM Amit Kapila wrote:
>
> On Fri, May 5, 2023 at 1:16 PM Drouvot, Bertrand
> wrote:
> >
> >
> > After multiple attempts, I got one failing one.
> >
> > Issue is that we expect this file to be removed:
> >
> > [07:24:2
re why yet.
>
Can you try to print the value returned by
XLogGetReplicationSlotMinimumLSN() in KeepLogSeg() on standby? Also,
please try to print "attempting to remove WAL segments ..." on the
primary. We can see, if by any chance some slot is holding us to
remove the required WAL file.
--
With Regards,
Amit Kapila.
On Fri, May 5, 2023 at 11:08 AM Drouvot, Bertrand
wrote:
>
> On 5/4/23 6:43 AM, Amit Kapila wrote:
> > On Thu, May 4, 2023 at 8:37 AM vignesh C wrote:
> >>
> >> Thanks for posting the updated patch, I had run this test in a loop of
> >> 100 times to veri
gt; +my $standby_walfile = $node_standby->data_dir . '/pg_wal/' . $walfile_name;
>
Thanks for the verification. I have pushed the patch.
--
With Regards,
Amit Kapila.
I have changed that and a comment in the
attached. I'll push this tomorrow unless there are further comments.
--
With Regards,
Amit Kapila.
v9-0001-Test-that-invalidated-logical-slots-doesn-t-retai.patch
Description: Binary data
On Tue, May 2, 2023 at 9:46 AM Amit Kapila wrote:
>
> On Tue, May 2, 2023 at 9:06 AM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Friday, April 28, 2023 2:18 PM Masahiko Sawada
> > wrote:
> > >
> > > >
> > > > Alexander, does the propo
On Mon, May 1, 2023 at 10:03 PM Robert Haas wrote:
>
> On Sat, Apr 22, 2023 at 7:06 AM Amit Kapila wrote:
> > I don't think introducing a GUC for this is a good idea. We can
> > directly output this message in the server log either at LOG or DEBUG1
> > level.
>
&g
ure by changing the DEBUG2 message in
RemoveOldXlogFiles() to LOG. In the case, where the test fails, it may
not remove the WAL file corresponding to an invalid slot whereas it
will remove the WAL file when the test succeeds.
--
With Regards,
Amit Kapila.
e);
Can we slightly reword this comment as: "The locks will be acquired
once the worker is initialized."?
--
With Regards,
Amit Kapila.
to a static variable
> before registering pa_shutdown() callback.
>
Why not simply move the registration of pa_shutdown() to someplace
after logicalrep_worker_attach()? BTW, it seems we don't have access
to MyLogicalRepWorker->leader_pid till we attach to the worker slot
via logicalrep_worker_attach(), so we anyway need to do what you are
suggesting after attaching to the worker slot.
--
With Regards,
Amit Kapila.
-id/CAB7nPqRX6w9UY%2B%3DOy2jqTVwi0hqT2y4%3DfUc7fNG4U-296JBvYQ%40mail.gmail.com
--
With Regards,
Amit Kapila.
On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada wrote:
>
> On Fri, Apr 28, 2023 at 11:51 AM Amit Kapila wrote:
> >
> > On Wed, Apr 26, 2023 at 4:11 PM Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Wednesday, April 26, 2023 5:00 PM Alexander Lakhin
ill on the primary");
How is it guaranteed that the WAL file corresponding to the
invalidated slot on standby will still be present on primary? Can you
please explain the logic behind this test a bit more like how the WAL
file switch helps you to achieve the purpose?
--
With Regards,
Amit Kapila.
you are facing?
Sawada-San, and others, do you see any better way to fix it than what
has been proposed?
--
With Regards,
Amit Kapila.
On Thu, Apr 27, 2023 at 4:07 PM Drouvot, Bertrand
wrote:
>
> On 4/27/23 11:53 AM, Amit Kapila wrote:
> > On Thu, Apr 27, 2023 at 2:16 PM Drouvot, Bertrand
> > wrote:
> >>
> >> On 4/27/23 10:11 AM, Yu Shi (Fujitsu) wrote:
> >>> Hi hackers,
> &g
On Thu, Apr 27, 2023 at 1:05 PM Drouvot, Bertrand
wrote:
>
> On 4/27/23 5:37 AM, Amit Kapila wrote:
> > On Wed, Apr 26, 2023 at 4:41 PM Drouvot, Bertrand
> > wrote:
> >
> > +When in recovery, the default value of target_lsn is $node->lsn('replay')
>
g regarding the test but looks more appropriate to
> me.
>
It may not make a difference as this is anyway an error case but it
looks more logical to LIMIT by 1 as you are fetching a single LSN
value and it will be consistent with other tests in this file and the
test case in the file 006_logical_decoding.pl.
BTW, in the same test, I see it uses wait_for_catchup() in one place
and wait_for_replay_catchup() in another place after Insert. Isn't it
better to use wait_for_replay_catchup in both places?
--
With Regards,
Amit Kapila.
ssed to
wait_for_subscription_sync()
+is a standby node.
I think this will be useful whenever wait_for_catchup has been called
for a standby node (where self is a standby node). I have tried even
by commenting wait_for_subscription_sync in the new test then it fails
for $node_standby->wait_for_catchup('tap_sub');. So instead, how about
a comment like: "When in recovery, the default value of target_lsn is
$node->lsn('replay') instead which ensures that the cascaded standby
has caught up to what has been replayed on the standby."?
--
With Regards,
Amit Kapila.
er possibility is to introduce a new
variable 'InitializingApplyWorker' similar to
'InitializingParallelWorker' and use that to prevent releasing locks.
--
With Regards,
Amit Kapila.
On Wed, Apr 26, 2023 at 12:01 PM Masahiko Sawada wrote:
>
> On Wed, Apr 26, 2023 at 2:56 PM Amit Kapila wrote:
> >
> > On Wed, Apr 26, 2023 at 10:01 AM Masahiko Sawada
> > wrote:
> > >
> > > On Tue, Apr 25, 2023 at 12:58 PM Zhijie Hou (Fujitsu)
>
ose DMLs that publication doesn't exist as it
uses a historic snapshot. So, why do we expect DDLs between Drop and
Create of publication should be replicated?
--
With Regards,
Amit Kapila.
On Mon, Apr 24, 2023 at 5:38 PM Drouvot, Bertrand
wrote:
>
> On 4/24/23 11:45 AM, Amit Kapila wrote:
> > On Mon, Apr 24, 2023 at 11:54 AM Amit Kapila
> > wrote:
> >>
> >> On Mon, Apr 24, 2023 at 11:24 AM Drouvot, Bertrand
>
On Mon, Apr 24, 2023 at 3:36 PM Drouvot, Bertrand
wrote:
>
> On 4/24/23 8:24 AM, Amit Kapila wrote:
>
> > 2.
> > +# Speed up the subscription creation
> > +$node_primary->safe_psql('postgres', "SELECT pg_log_standby_snapshot()");
> > +
&
On Mon, Apr 24, 2023 at 11:54 AM Amit Kapila wrote:
>
> On Mon, Apr 24, 2023 at 11:24 AM Drouvot, Bertrand
> wrote:
> >
>
> Few comments:
>
>
+# We can not test if the WAL file still exists immediately.
+# We need to let some time to the standby to actua
evel here. It is discussed in
an email [1]. I'll push this part tomorrow unless Andres or someone
else thinks that we still need this.
[1] - https://www.postgresql.org/message-id/523315.1681245505%40sss.pgh.pa.us
--
With Regards,
Amit Kapila.
c
+$node_primary->safe_psql('postgres', "SELECT pg_log_standby_snapshot()");
+$node_subscriber->wait_for_subscription_sync($node_standby, 'tap_sub');
It is not clear to me why you need to do pg_log_standby_snapshot() twice.
3. Why do you need $psql_subscriber to be used in a different way
instead of using safe_psql as is used for node_primary?
--
With Regards,
Amit Kapila.
On Fri, Apr 21, 2023 at 6:21 PM Robert Haas wrote:
>
> On Fri, Apr 21, 2023 at 8:19 AM Amit Kapila wrote:
> > LGTM. Let's see if Robert or others have any comments, otherwise, I'll
> > push this early next week.
>
> LGTM too.
>
Pushed.
--
With Regards,
Amit Kapila.
but otherwise, the changes look good to me.
--
With Regards,
Amit Kapila.
n see tthe more granular picture. How big
> of an effort is this?
>
Right, I think this is the key factor to decide whether we can get
this in PG16 or not. If this is just adding a new column and a few
existing stats update calls then it should be okay to get in but if
this requires some more complex work then we can probably update the
docs.
--
With Regards,
Amit Kapila.
On Mon, Apr 24, 2023 at 8:18 AM shveta malik wrote:
>
> On Thu, Apr 20, 2023 at 3:40 PM Amit Kapila wrote:
> >
> > On Thu, Apr 20, 2023 at 9:11 AM shveta malik wrote:
> > >
> > >
> > > Few comments for ddl_deparse.c in patch dated April17:
> >
in
> whether we should use FATAL or ERROR for this situation. The stream is
> not provided by user, and the session or process cannot continue.
>
I think ERROR should be fine here similar to other cases in worker.c.
--
With Regards,
Amit Kapila.
llel worker but fails due to insufficient worker slots.
>
I don't think introducing a GUC for this is a good idea. We can
directly output this message in the server log either at LOG or DEBUG1
level.
--
With Regards,
Amit Kapila.
ng the update
chain via heap_lock_updated_tuple() where we don't acquire the lock on
the tuple. See comments atop heap_lock_updated_tuple(). You can verify
if that is the case by adding some DEBUG logs in that function.
> At first, I thought both (1) and (2) were obstacles. However, I understood
> from your indication that (1) is not a bug.
> I would be grateful if you could also give me your opinion on (2).
>
If my above observation is correct then it is not a bug as it is
behaving as per the current design.
--
With Regards,
Amit Kapila.
On Fri, Apr 21, 2023 at 12:30 PM vignesh C wrote:
>
> On Fri, 21 Apr 2023 at 01:49, Robert Haas wrote:
> >
> > On Thu, Apr 20, 2023 at 1:08 AM Amit Kapila wrote:
> > > Pushed. I noticed that we didn't display this new subscription option
> > > 'pas
tSwitchTo(oldcxt);
> + }
> +}
>
> It will be good to add some comments in the 'else' part. It is not
> very much clear what exactly we are doing here and for which scenario.
>
Yeah, that would be better. I feel the event trigger related changes
should be moved to a separate patch in itself.
--
With Regards,
Amit Kapila.
So, we
can remove them. I see some references to 'O' in the comments, those
also need to be modified.
2.
+ /* For identity column, find the sequence owned by column in order
+ * to deparse the column definition */
In multi-line comments, the first and last lines should be empty.
Refer to multi-line comments at other places.
3.
+ return object_name.data;
+
+}
An extra empty line before } is not required.
--
With Regards,
Amit Kapila.
ch success and few reverts.
>
Congratulations to the new committers!
--
With Regards,
Amit Kapila.
On Mon, Apr 17, 2023 at 9:12 AM Masahiko Sawada wrote:
>
> On Fri, Apr 7, 2023 at 6:37 PM Amit Kapila wrote:
> >
> > On Thu, Apr 6, 2023 at 6:57 PM Masahiko Sawada
> > wrote:
> > >
> > >
> > > While writing a PoC patch, I found some dif
ly.
>
> 6) There are plenty of places where we use 'append_not_present'
> without using 'append_null_object'.
> Do we need to have 'append_null_object' along with
> 'append_not_present' at these places?
>
What is the difference if we add a null object or not before not_present?
--
With Regards,
Amit Kapila.
On Thu, Apr 13, 2023 at 8:02 AM Amit Kapila wrote:
>
> On Wed, Apr 12, 2023 at 5:50 PM Robert Haas wrote:
> >
> > On Tue, Apr 11, 2023 at 10:56 PM Amit Kapila
> > wrote:
> > > Anyway, I don't think as such there is any problem
> > > with restart
On Wed, Apr 12, 2023 at 4:53 PM Amit Kapila wrote:
>
>
> Few comments on 0001
> ===
>
Some more comments on 0001
==
1.
+/*
+ * Subroutine for CREATE TABLE/CREATE DOMAIN deparsing.
+ *
+ * Given a table OID or domain OID, obtain its constra
like that
> patch, or at least add a to the docs.
>
+1 to do one of the above. I think there is a good chance that
somebody might be doing more harm by using it so removing this
shouldn't be a problem. Personally, I have not heard of people using
it but OTOH it is difficult to predict so giving some time is also not
a bad idea.
Do others have any opinion/suggestion on this matter?
--
With Regards,
Amit Kapila.
On Wed, Apr 12, 2023 at 5:50 PM Robert Haas wrote:
>
> On Tue, Apr 11, 2023 at 10:56 PM Amit Kapila wrote:
> > Anyway, I don't think as such there is any problem
> > with restarting the worker even when the subscription owner is a
> > superuser, so adjusted th
nction is doing. Shall we expose that and use it?
7.
+static ObjTree *
+new_objtree(char *fmt)
+{
+ ObjTree*params;
+
+ params = palloc0(sizeof(ObjTree));
Here, the variable name params appear a bit odd. Shall we change it to
objtree or obj?
--
With Regards,
Amit Kapila.
On Tue, Apr 11, 2023 at 8:21 PM Robert Haas wrote:
>
> On Tue, Apr 11, 2023 at 5:53 AM Amit Kapila wrote:
> > I think additionally, we should check that the new owner of the
> > subscription is not a superuser, otherwise, anyway, this parameter is
> > ignored. Please
On Mon, Apr 10, 2023 at 9:15 PM Robert Haas wrote:
>
> On Sat, Apr 8, 2023 at 1:35 AM Amit Kapila wrote:
> > Do we need to have a check for this new option "password_required" in
> > maybe_reread_subscription() where we "Exit if any parameter that
> > af
er
> names(pg_deparse_trig_%s_%u),
> because they will be created along with create publication command. Referring
> to other built-in triggers(foreign key trigger), it has a tgisinternal catalog
> column which can be used to skip the dump for them.
>
> Personally, I think we can follow this style and add a isinternal column to
> pg_event_trigger and use it to skip the dump.
>
+1. This will not only help pg_dump but also commands like Alter Event
Trigger which enables/disables user-created event triggers but such
ops should be prohibited for internally created event triggers.
--
With Regards,
Amit Kapila.
sts or
something like that. Even for the Attach operation, I prefer (c) as
the other options don't seem logical to me and may add more complexity
to this work.
Thoughts?
--
With Regards,
Amit Kapila.
tion names to give that input.
--
With Regards,
Amit Kapila.
On Sun, Apr 9, 2023 at 11:33 AM Gregory Stark (as CFM)
wrote:
>
> On Fri, 7 Apr 2023 at 01:28, Amit Kapila wrote:
> >
> > On Wed, Apr 5, 2023 at 5:58 AM Peter Smith wrote:
> >
> > > PSA patches to add those tab completions.
> >
> > LGTM, so pushed.
&
On Fri, Apr 7, 2023 at 6:59 PM Masahiko Sawada wrote:
>
> On Fri, Apr 7, 2023 at 6:10 PM Amit Kapila wrote:
> >
> > On Fri, Apr 7, 2023 at 1:12 PM Masahiko Sawada
> > wrote:
> > >
> >
> > Do you mean that because 'password_required' is
option is
related to the remote connection so I thought it is worth considering
whether we want to exit and restart the apply worker when this option
is changed.
--
With Regards,
Amit Kapila.
On Sat, Apr 8, 2023 at 9:31 AM Andres Freund wrote:
>
> On 2023-04-08 09:15:05 +0530, Amit Kapila wrote:
> > The new approach for invalidation looks clean. BTW, I see minor
> > inconsistency in the following two error messages (errmsg):
>
> Thanks for checking.
>
t has been invalidated because it was conflicting
with recovery.")));
Won't it be better to keep the same errmsg in the above two cases?
--
With Regards,
Amit Kapila.
t the table list is not
> persisted. If the apply worker restarts during the initial table sync,
> it could not get the same list as before.
>
Agreed, this has some drawbacks. We can try to explore this if the
above idea of the new catalog table doesn't solve this problem.
--
With Regards,
Amit Kapila.
On Fri, Apr 7, 2023 at 1:12 PM Masahiko Sawada wrote:
>
> On Fri, Apr 7, 2023 at 2:28 PM Amit Kapila wrote:
> >
> > On Wed, Apr 5, 2023 at 5:58 AM Peter Smith wrote:
> > >
> >
> > LGTM, so pushed. BTW, while looking at this, I noticed that newly
&g
1201 - 1300 of 4051 matches
Mail list logo
