s will make the patch bit clean w.r.t
this new variable.
--
With Regards,
Amit Kapila.
replication_slots()? Can you please update the comments
for the same?
2.
@@ -36,6 +36,7 @@ generate_old_dump(void)
{
char sql_file_name[MAXPGPATH],
log_file_name[MAXPGPATH];
+
DbInfo*old_db = _cluster.dbarr.dbs[dbnum];
Spurious line change.
--
With Regards,
Amit Kapila.
On Tue, Aug 15, 2023 at 7:51 AM Masahiko Sawada wrote:
>
> On Mon, Aug 14, 2023 at 2:07 PM Amit Kapila wrote:
> >
> > On Mon, Aug 14, 2023 at 7:57 AM Masahiko Sawada
> > wrote:
> > > Another idea is (which might have already discussed thoguh) that we ch
On Thu, Aug 10, 2023 at 10:15 AM Amit Kapila wrote:
>
> On Wed, Aug 9, 2023 at 8:28 AM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Thursday, August 3, 2023 7:30 PM Melih Mutlu
> > wrote:
> >
> > > Right. I attached the v26 as you asked.
> >
&
r any other object. I understand that it could be convenient for
some use cases where some of the logical slots are not yet caught up
w.r.t WAL and users want to upgrade without the slots but not sure if
that is really the case. Does anyone else have an opinion on this
point?
--
With Regards,
Amit Kapila.
On Mon, Aug 14, 2023 at 7:57 AM Masahiko Sawada wrote:
>
> On Sat, Aug 12, 2023, 15:20 Amit Kapila wrote:
>>
>> I don't think we need the complexity of version-specific checks if we
>> do what we do in get_control_data(). Basically, invoke
>> version-specific
ut.
>
> 2a. IIUC that is all fine. The problem is that I think there should be
> exactly the same logic for the parallel apply workers here also.
>
Did you try to reproduce this condition, if not, can you please try it
once? I wonder if the leader worker crashed, won't it lead to a
restart of the server?
--
With Regards,
Amit Kapila.
in, I couldn't reproduce the cases where you saw significantly degraded
> performance.
>
I am not surprised to see that you don't see regression because as per
Vignesh's analysis, this is purely a timing issue where sometimes
after the patch the slot creation can take more time because there is
a constant inflow of transactions on the publisher. I think we are
seeing it because this workload is predominantly just creating and
destroying slots. We can probably improve it later as discussed
earlier by using a single for multiple copies (especially for small
tables) or something like that.
--
With Regards,
Amit Kapila.
On Fri, Aug 11, 2023 at 11:38 PM Bruce Momjian wrote:
>
> On Fri, Aug 11, 2023 at 10:46:31AM +0530, Amit Kapila wrote:
> > On Thu, Aug 10, 2023 at 7:07 PM Masahiko Sawada
> > wrote:
> > > What I imagined is that we do this check before
> > > check_and_dump_ol
On Sat, Aug 12, 2023 at 5:01 AM Peter Smith wrote:
>
> On Fri, Aug 11, 2023 at 9:13 PM Amit Kapila wrote:
> >
> > On Fri, Aug 11, 2023 at 3:41 PM Peter Smith wrote:
> > >
> > > On Fri, Aug 11, 2023 at 7:33 PM Amit Kapila
> > > wrote:
> > >
On Fri, Aug 11, 2023 at 3:41 PM Peter Smith wrote:
>
> On Fri, Aug 11, 2023 at 7:33 PM Amit Kapila wrote:
> >
> > On Thu, Aug 10, 2023 at 7:50 AM Peter Smith wrote:
> > >
> > > >
> > > > * If you do the above then there
of two new macros isTablesyncWorker() and
isLeaderApplyWorker() adds much value, so removed those and ran
pgindent. I am planning to commit this patch early next week unless
you or others have any comments.
--
With Regards,
Amit Kapila.
v6-0001-Simplify-determining-logical-replication-worker-t.patch
Description: Binary data
On Fri, Aug 11, 2023 at 10:43 AM Julien Rouhaud wrote:
>
> On Thu, Aug 10, 2023 at 04:30:40PM +0900, Masahiko Sawada wrote:
> > On Thu, Aug 10, 2023 at 2:27 PM Amit Kapila wrote:
> > >
> > > Sawada-San, Julien, and others, do you have any thoughts on the above
&g
On Thu, Aug 10, 2023 at 7:07 PM Masahiko Sawada wrote:
>
> On Thu, Aug 10, 2023 at 12:52 PM Amit Kapila wrote:
> >
> >
> > Are you suggesting doing this before we start the old cluster or after
> > we stop the old cluster? I was thinking about the pros and con
On Thu, Aug 10, 2023 at 5:07 PM John Naylor
wrote:
>
> On Thu, Aug 10, 2023 at 5:54 PM Michael Paquier wrote:
> >
> > On Thu, Aug 10, 2023 at 03:56:37PM +0530, Amit Kapila wrote:
> > > In MSVC build, on doing: perl mkvcbuild.pl after this commit, I am
&
...
undefined symbol: USE_LOONGARCH_CRC32C at src/include/pg_config.h line
718 at ../postgresql/src/tools/msvc/Mkvcbuild.pm line 872.
Am I missing something or did the commit miss something?
--
With Regards,
Amit Kapila.
On Mon, Aug 7, 2023 at 3:46 PM Amit Kapila wrote:
>
> On Mon, Aug 7, 2023 at 1:06 PM Julien Rouhaud wrote:
> >
> > On Mon, Aug 07, 2023 at 12:42:33PM +0530, Amit Kapila wrote:
> > > On Mon, Aug 7, 2023 at 11:29 AM Julien Rouhaud wrote:
> > > >
> >
ode even though the tablesync for a rel is in
progress which I guess could easily heart us for larger values of
max_logical_replication_workers. So, that could be another motivation
to think for a different locking scheme.
--
With Regards,
Amit Kapila.
On Thu, Aug 10, 2023 at 6:46 AM Masahiko Sawada wrote:
>
> On Wed, Aug 9, 2023 at 1:15 PM Amit Kapila wrote:
> >
> > On Wed, Aug 9, 2023 at 8:01 AM Masahiko Sawada
> > wrote:
> >
> > I feel it would be a good idea to provide such a tool for users to
>
transaction, we distribute invalidation messages to other
> concurrent transactions, like we do for snapshots. But we might not
> need to distribute all types of invalidation messages, though.
>
I would prefer the solution on these lines. It would be good if we
distribute only the required type of invalidations.
--
With Regards,
Amit Kapila.
ackets () exactly will address the translatability concern.
> DETAIL: The subscription that you are creating has the following
> publications containing tables written to by other subscriptions:
> "pub_s1_at_pub", "pub_all_at_pub"
>
Fair enough. Peter E., do let us know what you think makes sense here?
--
With Regards,
Amit Kapila.
the
variable name is_parallel_apply_worker in logicalrep_worker_launch.
--
With Regards,
Amit Kapila.
On Wed, Aug 9, 2023 at 8:20 AM Peter Smith wrote:
>
> On Tue, Aug 8, 2023 at 6:52 PM Amit Kapila wrote:
> >
> > On Tue, Aug 8, 2023 at 1:50 PM Peter Eisentraut
> > wrote:
> > >
> > > This patch added the following error message:
> > >
On Wed, Aug 9, 2023 at 8:01 AM Masahiko Sawada wrote:
>
> On Mon, Aug 7, 2023 at 6:02 PM Amit Kapila wrote:
> >
> > On Mon, Aug 7, 2023 at 2:02 PM Masahiko Sawada
> > wrote:
> > >
> > > WAL records for hint bit updates could be generated even in upgrad
ions
but if you have any questions related to how logical replication work
then I might be able to help.
--
With Regards,
Amit Kapila.
On Tue, Aug 8, 2023 at 1:50 PM Peter Eisentraut wrote:
>
> On 12.09.22 07:23, vignesh C wrote:
> > On Fri, 9 Sept 2022 at 11:12, Amit Kapila wrote:
> >>
> >> On Thu, Sep 8, 2022 at 9:32 AM vignesh C wrote:
> >>>
> >>>
>
On Mon, Aug 7, 2023 at 1:06 PM Julien Rouhaud wrote:
>
> On Mon, Aug 07, 2023 at 12:42:33PM +0530, Amit Kapila wrote:
> > On Mon, Aug 7, 2023 at 11:29 AM Julien Rouhaud wrote:
> > >
> > > Unless I'm missing something I don't see what prevents something
On Mon, Aug 7, 2023 at 2:02 PM Masahiko Sawada wrote:
>
> On Mon, Aug 7, 2023 at 12:54 PM Amit Kapila wrote:
> >
> > On Sun, Aug 6, 2023 at 6:02 PM Masahiko Sawada
> > wrote:
> > >
> > > IIUC the above query checks if the WAL record writt
On Mon, Aug 7, 2023 at 11:29 AM Julien Rouhaud wrote:
>
> On Mon, Aug 07, 2023 at 09:24:02AM +0530, Amit Kapila wrote:
> >
> > I think autovacuum is not enabled during the upgrade. See comment "Use
> > -b to disable autovacuum." in start_postmaster(). However
ore the upgrade
because anyway, those won't be usable after the upgrade.
Having said that, I think we need a flag for pg_dump to dump the slots.
--
With Regards,
Amit Kapila.
I think it should be based on commit_time because as far as I see we
can only get that on the client.
--
With Regards,
Amit Kapila.
the upgrade. See comment "Use
-b to disable autovacuum." in start_postmaster(). However, I am not
sure if there can't be any additional WAL from checkpointer or
bgwriter. Checkpointer has a code that ensures that if there is no
important WAL activity then it would be skipped. Similarly, bgwriter
also doesn't LOG xl_running_xacts unless there is an important
activity. I feel if there is a chance of any WAL activity during the
upgrade, we need to either change the check to ensure such WAL records
are expected or document the same in some way.
--
With Regards,
Amit Kapila.
e
are invalid slots in the old cluster; (c) provide a new function like
pg_copy_logical_replication_slot_contents() where we copy the required
contents like invalid status(ReplicationSlotInvalidationCause), etc.
Personally, I would prefer (b) because it will minimize the steps
required to perform by the user after the upgrade and looks cleaner
solution.
Thoughts?
--
With Regards,
Amit Kapila.
On Wed, Aug 2, 2023 at 8:20 AM Amit Kapila wrote:
>
> On Tue, Aug 1, 2023 at 12:11 PM Alvaro Herrera
> wrote:
> >
> > On 2023-Aug-01, Peter Smith wrote:
> >
> > > PSA a small patch making those above-suggested changes. The 'make
> > > check'
lsn. This means that the pg_upgrade fails if logical slots has caught up the
> current
> WAL location. IIUC DBA must do following steps:
>
> 1. shutdown old publisher
> 2. disable the subscription once <- this is mandatory, otherwise the
> walsender may
>send the record during the upgrade and confirmed_lsn may point the
> SHUTDOWN_CHECKPOINT
> 3. do pg_upgrade <- pg_get_wal_record_content() may raise an ERROR if 2. was
> skipped
>
But we have already seen that we write shutdown_checkpoint record only
after logical walsender is shut down. So, how above is possible?
--
With Regards,
Amit Kapila.
dify directly.
>
> One approach is adding an SQL funciton which set restart_lsn to aritrary value
> (or 0/0, invalid), but it seems dangerous.
>
I see your point related to WALAVAIL_REMOVED status of the slot but
did you test the scenario I have explained in my comment? Basically, I
want to know whether it can impact the user in some way. So, please
check whether the corresponding subscriptions will be allowed to drop.
You can test it both before and after the upgrade.
--
With Regards,
Amit Kapila.
On Thu, Aug 3, 2023 at 9:35 AM Peter Smith wrote:
>
> On Wed, Aug 2, 2023 at 11:19 PM Amit Kapila wrote:
> >
> > On Wed, Aug 2, 2023 at 4:09 PM Melih Mutlu wrote:
> > >
> > > PFA an updated version with some of the earlier reviews addressed.
> > >
few of those. If you guys are okay with this then let's commit it and
then we can focus more on the remaining patches.
[1] -
https://www.postgresql.org/message-id/CAHut%2BPs3Du9JFmhecWY8%2BVFD11VLOkSmB36t_xWHHQJNMpdA-A%40mail.gmail.com
--
With Regards,
Amit Kapila.
v25-0001-Refactor-to
> am_XXX_worker() is used. If implementing this leads to something like
> the above with is_worker_type, -1 from me.
>
What I was suggesting to Peter to have something like:
static inline bool
am_tablesync_worker(void)
{
return (MyLogicalRepWorker->type == TYPE_APPLY_WORKER);
}
--
With Regards,
Amit Kapila.
ng &&
!pq_is_send_pending())
break;
..
}
Now, it seems that in 0003 patch, instead of resetting flags
streamingDoneSending, and streamingDoneReceiving before start
replication, we should reset before create logical slots because we
need to read the WAL during that time as well to find the consistent
point.
--
With Regards,
Amit Kapila.
if we insist to pass MyLogicalRepWorker everywhere from the callers in
> worker.c / tablesync.c / applyparallelworker.c.
>
Agreed but having a dual set of macros is also not clean. Can we
provide only a unique set of inline functions instead?
--
With Regards,
Amit Kapila.
hich I am not
sure is a good idea to do in proc_exit() callbacks. We may want to
evaluate whether moving the suggested checks to proc_exit or any other
callback is a better idea. What do you think?
--
With Regards,
Amit Kapila.
On Tue, Aug 1, 2023 at 2:06 PM Masahiko Sawada wrote:
>
> On Tue, Aug 1, 2023 at 11:33 AM Amit Kapila wrote:
> >
> > On Mon, Jul 31, 2023 at 8:46 PM Masahiko Sawada
> > wrote:
> > >
> > > While reading the code, I realized that the following
"AND temporary = false AND wal_status IN ('reserved', 'extended');");
I think this can unnecessarily lead to reading a lot of WAL data if
the confirmed_flush_lsn for a slot is too much behind. Can we think of
improving this by passing the number of records to read which in this
case should be 1?
--
With Regards,
Amit Kapila.
On Mon, Jul 31, 2023 at 5:04 PM Tomas Vondra
wrote:
>
> On 7/31/23 11:25, Amit Kapila wrote:
> > On Sat, Jul 29, 2023 at 5:53 PM Tomas Vondra
> > wrote:
> >>
> >> On 7/28/23 14:44, Ashutosh Bapat wrote:
> >>> On Wed, Jul 26, 2023 at 8:48 PM To
(or subtransaction) from the following
comment: "Pick the largest transaction (or subtransaction) and evict
it from memory by streaming, if possible. Otherwise, spill to disk."?
Then by referring to streaming/spill-to-disk cases, one can understand
in which cases only top-level xacts are involved and in which cases
both are involved.
--
With Regards,
Amit Kapila.
e that you are seeing something else but if so then
please try to share a more concrete example.
--
With Regards,
Amit Kapila.
eturn isLeaderApplyWorker(MyLogicalRepWorker);
}
--
With Regards,
Amit Kapila.
uild a full
snapshot because we use that for copying the table but why do we need
to build that for copying the sequence especially when we directly
copy it from the sequence relation without caring for any snapshot?
--
With Regards,
Amit Kapila.
On Fri, Jul 28, 2023 at 6:12 PM Tomas Vondra
wrote:
>
> On 7/28/23 11:42, Amit Kapila wrote:
> > On Wed, Jul 26, 2023 at 8:48 PM Tomas Vondra
> > wrote:
> >>
> >> On 7/26/23 09:27, Amit Kapila wrote:
> >>> On Wed, Jul 26, 2023 at 9:37 A
On Wed, Jul 26, 2023 at 8:48 PM Tomas Vondra
wrote:
>
> On 7/26/23 09:27, Amit Kapila wrote:
> > On Wed, Jul 26, 2023 at 9:37 AM Amit Kapila wrote:
>
> Anyway, I was thinking about this a bit more, and it seems it's not as
> difficult to use the page LSN to ensure sequenc
need to restart the sync slot
worker to allow this information to be sent to the primary. We do
something similar for apply worker in logical replication.
--
With Regards,
Amit Kapila.
On Wed, Jul 26, 2023 at 10:31 AM shveta malik wrote:
>
> On Mon, Jul 24, 2023 at 9:00 AM Amit Kapila wrote:
> >
> > On Mon, Jul 24, 2023 at 8:03 AM Bharath Rupireddy
> > wrote:
> > >
> > > Is having one (or a few more - not
> > > necessaril
on here.
> ~
>
> 1b.
> Since this is a new function, should it be named according to the
> convention for static functions?
>
> e.g.
> ApplicationNameForTablesync -> app_name_for_tablesync
>
I think for now let's follow the style for similar functions like
ReplicationOriginNameForLogicalRep() and
ReplicationSlotNameForTablesync().
--
With Regards,
Amit Kapila.
On Wed, Jul 26, 2023 at 9:37 AM Amit Kapila wrote:
>
> On Tue, Jul 25, 2023 at 5:29 PM Tomas Vondra
> wrote:
> >
> > On 7/25/23 08:28, Amit Kapila wrote:
> > > On Mon, Jul 24, 2023 at 9:32 PM Tomas Vondra
> > > wrote:
> > >>
> > >
ing of existing
variables but I feel it would be better to do as a separate patch
along with improving the consistency function names. For new
functions/variables, it would be good to follow a consistent style.
--
With Regards,
Amit Kapila.
On Tue, Jul 25, 2023 at 10:33 PM Andres Freund wrote:
>
> On 2023-07-25 14:31:00 +0530, Amit Kapila wrote:
> > To ensure that all the data has been sent during the upgrade, we can
> > ensure that each logical slot's confirmed_flush_lsn (position in the
> > WAL till which
On Tue, Jul 25, 2023 at 5:29 PM Tomas Vondra
wrote:
>
> On 7/25/23 08:28, Amit Kapila wrote:
> > On Mon, Jul 24, 2023 at 9:32 PM Tomas Vondra
> > wrote:
> >>
> >> On 7/24/23 12:40, Amit Kapila wrote:
> >>> On Wed, Jul 5, 2023 at 8:21 PM Ash
On Mon, Jul 24, 2023 at 6:14 PM Ashutosh Bapat
wrote:
>
> On Sat, Jul 22, 2023 at 10:18 AM Amit Kapila wrote:
>
> > Right. I'll push and backpatch this till 15 by Tuesday unless you guys
> > think otherwise.
>
> WFM.
>
Pushed.
--
With Regards,
Amit Kapila.
On Mon, Jul 24, 2023 at 4:22 PM Tomas Vondra
wrote:
>
> On 7/24/23 12:40, Amit Kapila wrote:
> > On Wed, Jul 5, 2023 at 8:21 PM Ashutosh Bapat
> > wrote:
> >>
> >> 0005, 0006 and 0007 are all related to the initial sequence sync. [3]
> >> resulted in
] - https://www.postgresql.org/docs/devel/pgupgrade.html
--
With Regards,
Amit Kapila.
On Mon, Jul 24, 2023 at 9:32 PM Tomas Vondra
wrote:
>
> On 7/24/23 12:40, Amit Kapila wrote:
> > On Wed, Jul 5, 2023 at 8:21 PM Ashutosh Bapat
> > wrote:
> >
> > Even after that, see below the value of the sequence is still not
> > caught up. Later, when th
ect nextval('s');
nextval
-----
67
(1 row)
--
With Regards,
Amit Kapila.
we
don't set XLOG_INCLUDE_ORIGIN while logging sequence values.
--
With Regards,
Amit Kapila.
is a possibility of a large
gap in syncing the slots which probably means we need to retain
corresponding WAL for a much longer time on the primary. If we can
prove that the gap won't be large enough to matter then this would be
probably worth considering otherwise, I think we should find a way to
scale the number of workers to avoid the large gap.
--
With Regards,
Amit Kapila.
On Mon, Jul 24, 2023 at 6:39 AM Masahiko Sawada wrote:
>
> On Sat, Jul 22, 2023 at 7:32 PM Amit Kapila wrote:
> >
> >
> > You have moved most of the comments related to the restriction of
> > which index can be picked atop IsIndexUsableForReplicaIdentityFull().
ds further investigation.
>
> > * Can you document the interaction between locale keywords
> > ("@colStrength=primary") and a rule like '[strength 2]'?
>
> I'll look into that.
>
This thread is listed on PostgreSQL 16 Open Items list. This is a
gentle reminder to see if there is a plan to move forward with respect
to open points.
--
With Regards,
Amit Kapila.
ot be a good idea in some
cases". Shall we move these as well atop
IsIndexUsableForReplicaIdentityFull()?
--
With Regards,
Amit Kapila.
On Fri, Jul 21, 2023 at 6:28 AM Masahiko Sawada wrote:
>
> On Fri, Jul 21, 2023 at 1:39 AM Ashutosh Bapat
> wrote:
> >
> > On Thu, Jul 20, 2023 at 9:11 AM Amit Kapila wrote:
> > > >
> > > > Okay, changed it accordingly.
> > > >
> &
On Fri, Jul 21, 2023 at 12:05 PM Peter Smith wrote:
>
> On Fri, Jul 21, 2023 at 3:39 PM Amit Kapila wrote:
> >
> > On Fri, Jul 21, 2023 at 7:30 AM Peter Smith wrote:
> > >
> > > ~~~
> > >
> > > 2. StartLogRepWorker
> > >
> &g
criptionAndExit(void);
> +extern void StartLogRepWorker(int worker_slot);
>
> This placement (esp. with the missing whitespace) seems to be grouping
> the set_stream_options with the other 'pa' externs, which are all
> under the comment "/* Parallel apply worker setup and interactions
> */".
>
> Putting all these up near the other "extern void
> InitializeLogRepWorker(void)" might be less ambiguous.
>
+1. Also, note that they should be in the same order as they are in .c files.
--
With Regards,
Amit Kapila.
he HEAD
>> code used to do.
>>
>> It seems this mistake was introduced in patch v20-0001.
>
>
> I'm a bit confused here. Isn't it decided to use "logical replication worker"
> regardless of the worker's type [1]. That's why I made this change. If that's
> not the case here, I'll put it back.
>
I feel where the worker type is clear, it is better to use it unless
the same can lead to translation issues.
--
With Regards,
Amit Kapila.
egment pg_wal/00010001 has already
> been removed
> ```
>
> pg_upgrade runs pg_dump and then pg_resetwal, so dumping slots must be done
> separately to avoid above error.
>
Okay, so the point is that if we create the slot in the new cluster
before pg_resetwal then its restart_lsn will be set to the current LSN
position which will later be reset by pg_resetwal. So, we won't be
able to use such a slot, right?
--
With Regards,
Amit Kapila.
ent behavior keeps the trigger-firing rules
the same for all kinds of triggers which has merits but OTOH it causes
inconvenience to users, especially for foreign-key checks.
--
With Regards,
Amit Kapila.
t; > specific field. I'm okay with changing the name but feel like
> > relsync_completed would be misleading.
>
> My reading of the code is slightly different: Only these fields have
> the prefix ‘rel’ and they are all grouped under the comment “/* Used
> for initial table synchronization. */” because AFAIK only these fields
> are TWS specific (not used for other kinds of workers).
>
> Since this new flag field is also TWS-specific, therefore IMO it
> should follow the same consistent name pattern. But, if you are
> unconvinced, maybe see later if others have an opinion about it.
>
+1 to use the prefix 'rel' here as the sync is specific to the
relation. Even during apply phase, we will apply the relation-specific
changes. See should_apply_changes_for_rel().
--
With Regards,
Amit Kapila.
On Thu, Jul 20, 2023 at 9:10 AM Amit Kapila wrote:
>
> On Wed, Jul 19, 2023 at 10:08 AM Ashutosh Bapat
> wrote:
> >
> > On Wed, Jul 19, 2023 at 9:01 AM Amit Kapila wrote:
> > >
> > > On Tue, Jul 18, 2023 at 10:27 AM Masahiko Sawada
> > > wrote:
On Wed, Jul 19, 2023 at 10:08 AM Ashutosh Bapat
wrote:
>
> On Wed, Jul 19, 2023 at 9:01 AM Amit Kapila wrote:
> >
> > On Tue, Jul 18, 2023 at 10:27 AM Masahiko Sawada
> > wrote:
> > >
> > > Or can we use snprintf() writing "??? (%d)" to a fi
different
>
> FWIW, the former is bottlenecked by the number of WAL insertion locks, the
> second is bottlenecked by copying WAL into buffers due to needing to flush
> them.
>
This gives a better idea of what's going on. +1 for separating these waits.
--
With Regards,
Amit Kapila.
TM.
>>
>
> Overall looks good to me as well. Please consider the following as an
> optional improvement.
>
Pushed. Thanks for looking into this.
--
With Regards,
Amit Kapila.
be helpful anyway
> if the function comment can emphasize this function is shared by
> different worker types. e.g. "Common function to run the apply
> loop..."
>
I would prefer to change the comments as suggested by you in 4b
because both the workers (apply and tablesync) need to perform apply,
so it seems logical for both of them to invoke start_apply.
--
With Regards,
Amit Kapila.
On Tue, Jul 18, 2023 at 10:27 AM Masahiko Sawada wrote:
>
> On Tue, Jul 18, 2023 at 12:15 PM Amit Kapila wrote:
> >
> > On Mon, Jul 17, 2023 at 7:54 PM Alvaro Herrera
> > wrote:
> > >
> >
> > I have tried to check whether we have such usage in an
es till 0003.
4. In 0001's commit message, you can say that it will help the
upcoming reuse tablesync worker patch.
--
With Regards,
Amit Kapila.
t then the code
remains consistent and the backpatching would be easier.
--
With Regards,
Amit Kapila.
On Mon, Jul 17, 2023 at 6:19 PM Amit Kapila wrote:
>
> On Fri, Jun 30, 2023 at 7:29 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > I have analyzed more, and concluded that there are no difference between
> > manual
> > and shutdown checkpoint.
> >
> &
thing in the original patch you submitted, to ensure all
> these strings are compile-time constants; that's likely the most robust.
>
So will we be okay with something like the below:
ERROR: invalid logical replication message type "??? (88)"
CONTEXT: processing remote data for replic
ed a special check
as you have in patch 0003 or at least it doesn't seem to be required
in all cases.
[1] -
/*
* When SIGUSR2 arrives, we send any outstanding logs up to the
* shutdown checkpoint record (i.e., the latest record), wait for
* them to be replicated to the standby, and exit. ...
*/
--
With Regards,
Amit Kapila.
re is no default operator class, usually the type does not have an
>> equality operator.
>>
This sounds a bit generic to me. If required, we can give an example
so that it is easier to understand. But OTOH, I see that we use
"default operator class" in the docs and error messages, so this
should be probably okay.
--
With Regards,
Amit Kapila.
s been
consumed for a slot except for shutdown_checkpoint in patch 0003 but
do we need to think of any interaction with restart_lsn
(MyReplicationSlot->data.restart_lsn) which is the start point to read
WAL for decoding by walsender?
--
With Regards,
Amit Kapila.
On Sat, Jul 15, 2023 at 7:16 PM Euler Taveira wrote:
>
> On Sat, Jul 15, 2023, at 4:27 AM, Amit Kapila wrote:
>
> Do you have something like attached in mind?
>
>
> WFM. I would change the comment that says
>
> This function is called to provide context in the error
On Fri, Jul 14, 2023 at 3:07 PM Melih Mutlu wrote:
>
> Amit Kapila , 14 Tem 2023 Cum, 11:11 tarihinde şunu
> yazdı:
>>
>> Yeah, it is quite surprising that Design#2 is worse than master. I
>> suspect there is something wrong going on with your Design#2 patch.
>
On Tue, Jul 11, 2023 at 1:36 PM Masahiko Sawada wrote:
>
> On Thu, Jul 6, 2023 at 6:28 PM Amit Kapila wrote:
> >
> > One point to note is that the user may also get confused if the actual
> > ERROR says message type as 'X' and the context says '???'. I feel in
> > t
] -
- * We also skip indexes if the remote relation does not contain the leftmost
- * column of the index. This is because in most such cases sequential scan is
- * favorable over index scan.
--
With Regards,
Amit Kapila.
ined for it."?
>
> Thanks for discussing and giving suggestions. But it seems that the first
> sentence is difficult to read for me. How about attached?
>
The last line seems repetitive to me. So, I have removed it. Apart
from that patch looks good to me. Sergie, Peter, and others
dle any remote messages and client variable stuff during execution,
> although it could take more time to finsh the test.
>
I agree that this second approach can take more time but it would be
good to avoid special-purpose code the first approach needs. BTW, can
we try to evaluate the time difference between both approaches?
Anyway, in the first approach also, we need to run the test statement
twice.
--
With Regards,
Amit Kapila.
On Thu, Jul 13, 2023 at 8:07 PM Önder Kalacı wrote:
>
> Looks good to me. I have also done some testing, which works as
> documented/expected.
>
Thanks, I have pushed this after minor changes in the comments.
--
With Regards,
Amit Kapila.
> set soon.
>
Yeah, that would be helpful.
--
With Regards,
Amit Kapila.
point or box) that don't have a default operator class
for Btree or Hash. This won't be a problem if the table has a primary
key or replica identity defined for it."?
--
With Regards,
Amit Kapila.
On Thu, Jul 13, 2023 at 8:51 AM Amit Kapila wrote:
>
> On Wed, Jul 12, 2023 at 8:14 PM Önder Kalacı wrote:
> >
> >>
> >> > - return is_btree && !is_partial && !is_only_on_expression;
> >> > + /* Check wheth
mRoutine *amroutine = GetIndexAmRoutineByAmId(indexInfo->ii_Am,
+ false);
+ Assert(amroutine->amgettuple != NULL);
+ }
+#endif
Apart from having an assert, we have the following two options (a)
check this condition as well and return false if am doesn't support
amgettuple (b) report elog(ERROR, ..) in this case.
I am of the opinion that we should either have an assert for this or
do (b) because if do (a) currently there is no case where it can
return false. What do you think?
--
With Regards,
Amit Kapila.
o you think?
>
I think this is a valid concern. Can't we move all the checks
(including the remote attrs check) inside
IsIndexUsableForReplicaIdentityFull() and then call it from both
places? Won't we have attrmap information available in the callers of
FindReplTupleInLocalRel() via ApplyExecutionData?
--
With Regards,
Amit Kapila.
901 - 1000 of 6079 matches
Mail list logo
