Hi,
Thanks for the explanation.
As described in the commit message, we assume that extensions use the
> hook in a similar way to FDWs
I'm not sure if it is fair to assume that extensions use any hook in any
way.
So my question is: does the Citus extension use the hook like this?
> (Sorry, I
Hi Etsuro,
Thanks for the response!
> Maybe we could do so by leaving to extensions the decision whether
> they replace joins with pseudoconstant clauses, but I am not sure that
> that is a good idea, because that would require the authors to modify
> and recompile their extensions to fix the
Hi Etsuro, all
The commit[1] seems to break some queries in Citus[2], which is an
extension which relies on set_join_pathlist_hook.
Although the comment says */*Finally, give extensions a chance to
manipulate the path list.*/ *we use it to extract lots of information
about the joins and do the
Hi Masahiko, Amit, all
I've updated the patch.
>
I think the flow is much nicer now compared to the HEAD. I really don't
have any
comments regarding the accuracy of the code changes, all looks good to me.
Overall, I cannot see any behavioral changes as you already alluded to.
Maybe few minor
Hi,
> I've attached a draft patch for this idea.
I think it needs a rebase after edca3424342da323499a1998d18a888283e52ac7.
Also, as discussed in [1], I think one improvement we had was to
keep IsIndexUsableForReplicaIdentityFull() in a way that it is easier to
read & better documented in the
Hi,
>
> > The last line seems repetitive to me. So, I have removed it. Apart
> > from that patch looks good to me. Sergie, Peter, and others, any
> > thoughts?
>
> The v5 patch LGTM.
>
>
Overall looks good to me as well. Please consider the following as an
optional improvement.
My only minor
Hi Amit, Hayato, all
> +1 for the readability. I think we can as you suggest or I feel it
> > would be better if we return false as soon as we found any condition
> > is false. The current patch has a mixed style such that for
> > InvalidStrategy, it returns immediately but for others, it does a
Hi Hayato, all
> > - return is_btree && !is_partial && !is_only_on_expression;
> > + /* Check whether the index is supported or not */
> > + is_suitable_type = ((get_equal_strategy_number_for_am(indexInfo->ii_Am)
> > + != InvalidStrategy));
> > +
> > + is_partial = (indexInfo->ii_Predicate !=
Hi Amit, all
Amit Kapila , 12 Tem 2023 Çar, 13:09 tarihinde
şunu yazdı:
> On Wed, Jul 12, 2023 at 12:31 PM Masahiko Sawada
> wrote:
> >
> > On Tue, Jul 11, 2023 at 5:31 PM Peter Smith
> wrote:
> > >
> >
> > I don't think we have concluded any action for it. I agree that
> >
Hi,
I also think so. If this is true, how can we think of supporting
> indexes other than hash like GiST, and SP-GiST as mentioned by you in
> your latest email? As per my understanding if we don't have PK or
> replica identity then after the index scan, we do tuples_equal which
> will fail for
Hi Hayato,
> BTW, I have doubt that the restriction is not related with your commit.
> In other words, if the table has attributes which the datatype is not for
> operator
> class of Btree, we could not use REPLICA IDENTITY FULL. IIUC it is not
> documented.
> Please see attched script to
Hi Hayato, all
>
>
> This is a follow-up thread of [1]. The commit allowed subscribers to use
> indexes
> other than PK and REPLICA IDENTITY when REPLICA IDENTITY is FULL on
> publisher,
> but the index must be a B-tree. In this proposal, I aim to extend this
> functionality to allow
> for hash
Hi Shi Yu,
>
> Is there any reasons why we drop column here? Dropped column case has been
> tested on table dropped_cols. The generated column problem can be detected
> without dropping columns on my machine.
>
We don't really need to, if you check the first patch, we don't have DROP
for
Hi Shi Yu,
>
>
> 1.
> $node_publisher->safe_psql(
> 'postgres', qq(
> ALTER TABLE dropped_cols DROP COLUMN b_drop;
> + ALTER TABLE generated_cols DROP COLUMN b_gen;
> ));
> $node_subscriber->safe_psql(
> 'postgres', qq(
> ALTER
Hi Amit, Shi Yu
Now attaching the similar patches for generated columns.
Thanks,
Onder KALACI
Amit Kapila , 21 Mar 2023 Sal, 09:07 tarihinde
şunu yazdı:
> On Mon, Mar 20, 2023 at 6:28 PM Amit Kapila
> wrote:
> >
> > On Mon, Mar 20, 2023 at 2:58 PM Önder Kalacı
> wrote
Hi Shi Yu, all
Thanks for updating the patches. It seems you forgot to attach the patches
> of
> dropped columns for HEAD and pg15, I think they are the same as v2.
>
>
Yes, it seems I forgot. And, yes they were the same as v2.
> On HEAD, we can re-use clusters in other test cases, which can
Hi Shi Yu,
Thanks for the review, really appreciate it!
> I couldn't apply
> v2-0001-Ignore-dropped-columns-HEAD-REL_15-REL_14-REL_13.patch
> cleanly in v13 and v14. It looks the patch needs some changes in these
> versions.
>
>
> ```
> Checking patch src/backend/executor/execReplication.c...
Hi Amit, all
> You can first submit the fix for dropped columns with patches till
> v10. Once that is committed, then you can send the patches for
> generated columns.
>
>
Alright, attaching 2 patches for dropped columns, the names of the files
shows which
versions the patch can be applied to:
Thu, Mar 16, 2023 5:23 PM Amit Kapila wrote:
> >
> > On Mon, Mar 13, 2023 at 6:26 PM Önder Kalacı
> > wrote:
> > >
> > > Attaching v2
> > >
> >
> > Can we change the comment to: "Ignore dropped and generated columns as
> > the
>
>
>
> Thanks for the updated patch.
> Few minor comments:
> 1) The extra line break after IsIndexOnlyOnExpression function can be
> removed:
>
removed
>
>
> 2) Generally we don't terminate with "." for single line comments
> +
> + /*
> + * Simple case, we already have a primary key or a
Amit Kapila , 14 Mar 2023 Sal, 11:59 tarihinde
şunu yazdı:
> On Tue, Mar 14, 2023 at 12:48 PM Önder Kalacı
> wrote:
> >>
> >> 2.
> >> +# make sure that the subscriber has the correct data after the update
> UPDATE
> >>
> >> "up
Hi Amit, all
Amit Kapila , 14 Mar 2023 Sal, 09:50 tarihinde
şunu yazdı:
> On Mon, Mar 13, 2023 at 7:46 PM Önder Kalacı
> wrote:
> >
> > Attaching v47.
> >
>
> I have made the following changes in the attached patch (a) removed
> the function IsIdxSafeToSk
Hi Shi Yu,
> in RemoteRelContainsLeftMostColumnOnIdx():
>
> + if (indexInfo->ii_NumIndexAttrs < 1)
> + return false;
>
> Did you see any cases that the condition is true? I think there is at
> least one
> column in the index. If so, we can use an Assert().
>
Actually, it was
Hi Amit, all
> >
> > For that test, my goal was to ensure/show that the invalidation callback
> > is triggered after `DROP / CREATE INDEX` commands.
> >
>
> Fair point. I suggest in that case just keep one of the tests for Drop
> Index such that after that it will pick up a sequence scan.
Hi Shi Yu,
> 1.
> @@ -243,6 +243,17 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot
> *slot2,
> Form_pg_attribute att;
> TypeCacheEntry *typentry;
>
> +
> + Form_pg_attribute attr =
> TupleDescAttr(slot1->tts_tupleDescriptor, attrnum);
> +
>
>
Hi Amit, Peter, all
> > If the reason for the stats polling was only to know if some index is
> > chosen or not, I was wondering if you can just convey the same
> > information to the TAP test via some conveniently placed (DEBUG?)
> > logging.
> >
>
> I had thought about it but didn't convince
Hi Hou zj, Shi-san, all
> In this function, it used the local column number(keycol) to match the
> remote
> column number(attkeys), I think it will cause problem if the column order
> between pub/sub doesn't match. Like:
>
> ---
> - pub
> CREATE TABLE test_replica_id_full (x int, y int);
>
Hi Shi Yu,
> > >
> > >
> > > Reading [1], I think I can follow what you suggest. So, basically,
> > > if the leftmost column is not filtered, we have the following:
> > >
> > >> but the entire index would have to be scanned, so in most cases the
> planner
> > would prefer a sequential table
Hi Amit, all
> >> I think we can add such a test (which relies on existing buggy
> >> behavior) later after fixing the existing bug. For now, it would be
> >> better to remove that test and add it after we fix dropped columns
> >> issue in HEAD.
> >
> >
> > Alright, when I push the next version
Hi Amit, all
>
> 1.
> +# Testcase start: SUBSCRIPTION USES INDEX WITH PUB/SUB DIFFERENT DATA VIA
> +# A UNIQUE INDEX THAT IS NOT PRIMARY KEY OR REPLICA IDENTITY
>
> No need to use Delete test separate for this.
>
Yeah, there is really no difference between update/delete for this patch,
so it
Hi Amit, all
> > This triggers tuples_equal to fail. To fix that, I improved the
> tuples_equal
> > such that it skips the dropped columns.
> >
>
> By any chance, have you tried with generated columns?
Yes, it shows the same behavior.
> See
>
Hi all,
(cc'ed Amit as he has the context)
While working on [1], I realized that on HEAD there is a problem with the
$subject. Here is the relevant discussion on the thread [2]. Quoting my
own notes on that thread below;
I realized that the dropped columns also get into the tuples_equal()
>
Hi Amit, all
> I'll look for more opportunities and reply to the thread. I wanted to send
> this mail so that you can have a look at (1) earlier.
>
>
> I merged SUBSCRIPTION CREATE/DROP INDEX WORKS WITHOUT ISSUES
into SUBSCRIPTION CAN USE INDEXES WITH EXPRESSIONS AND COLUMNS.
Also, merged
Hi Amit, all
> wip_for_optimize_index_column_match
> +static bool
> +IndexContainsAnyRemoteColumn(IndexInfo *indexInfo,
> + LogicalRepRelation *remoterel)
> +{
> + for (int i = 0; i < indexInfo->ii_NumIndexAttrs; i++)
> + {
>
> Wouldn't it be better to just check if the first column is not
Hi Peter, all
> src/backend/replication/logical/relation.c
>
> 1. FindUsableIndexForReplicaIdentityFull
>
> +static Oid
> +FindUsableIndexForReplicaIdentityFull(Relation localrel)
> +{
> + List*indexlist = RelationGetIndexList(localrel);
> + Oid usableIndex = InvalidOid;
> + ListCell *lc;
Hi Amit,
> >
> >>
> >> 4.
> >> +# Testcase start: SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN
> >>
> >> As we have removed enable_indexscan check, you should remove this test.
> >
> >
> > Hmm, I think my rebase problems are causing confusion here, which v38
> fixes.
> >
>
> I think it is still
Hi,
Amit Kapila , 8 Mar 2023 Çar, 14:42 tarihinde şunu
yazdı:
> On Wed, Mar 8, 2023 at 4:51 PM Önder Kalacı wrote:
> >
> >
> >>
> >> I just share this case and then we
> >> can discuss should we pick the index which only contain the extra
> columns
Hi Amit, all
> >
>
> This new test takes ~9s on my machine whereas most other tests in
> subscription/t take roughly 2-5s. I feel we should try to reduce the
> test timing without sacrificing much of the functionality or code
> coverage.
Alright, that is reasonable.
> I think if possible we
Hi Vignesh C,
> > Hmm, can you please elaborate more on this? The declaration
> > and assignment are already on different lines.
> >
> > ps: pgindent changed this line a bit. Does that look better?
>
> I thought of changing it to something like below:
> bool isUsableIndex;
> Oid idxoid =
Hi Peter,
> 1.
> In my previous review [1] (comment #1) I wrote that only some of the
> "should" were misleading and gave examples where to change. But I
> didn't say that *every* usage of that word was wrong, so your global
> replace of "should" to "must" has modified a couple of places in
>
Hi Vignesh C,
>
> Few comments
> 1) Maybe this change is not required:
> fallback if no other solution is possible. If a replica identity other
> than full is set on the publisher side, a replica
> identity
> - comprising the same or fewer columns must also be set on the subscriber
>
Hi Shi Yu, all
> e.g.
> -- pub
> CREATE TABLE test_replica_id_full (x int, y int);
> ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL;
> CREATE PUBLICATION tap_pub_rep_full FOR TABLE test_replica_id_full;
> -- sub
> CREATE TABLE test_replica_id_full (x int, y int, z int);
> CREATE INDEX
Hi Hou zj, all
>
> IIRC, it invokes tuples_equal for all cases unless we are using replica
> identity key or primary key to scan. But there seem some other cases where
> the
> tuples_equal looks unnecessary.
>
> For example, if the table on subscriber don't have a PK or RI key but have
> a
>
Hi Peter, all
>
> 1.
> Saying the index "should" or "should not" do this or that sounds like
> it is still OK but just not recommended. TO remove this ambigity IMO
> most of the "should" ought to be changed to "must" because IIUC this
> patch will simply not consider indexes which do not obey
Hi Amit, all
> You have not given complete steps to reproduce the problem where
> instead of the index scan, a sequential scan would be picked. I have
> tried to reproduce by extending your steps but didn't see the problem.
> Let me know if I am missing something.
>
I think the steps you shared
Hi Amit, Peter
> > > Let me give an example to demonstrate why I thought something is fishy
> here:
> > >
> > > Imagine rel has a (non-default) REPLICA IDENTITY with Oid=.
> > > Imagine the same rel has a PRIMARY KEY with Oid=.
> > >
>
Hmm, alright, this is syntactically possible, but
Hi Amit, all
>
> Few comments:
> =
> 1.
> +get_usable_indexoid(ApplyExecutionData *edata, ResultRelInfo *relinfo)
> {
> ...
> + if (targetrelkind == RELKIND_PARTITIONED_TABLE)
> + {
> + /* Target is a partitioned table, so find relmapentry of the partition */
> + TupleConversionMap
Hi Shi Yu, all
> Thanks for updating the patch. Here are some comments on v33-0001 patch.
>
> 1.
> + if (RelationReplicaIdentityFullIndexScanEnabled(localrel) &&
> + remoterel->replident == REPLICA_IDENTITY_FULL)
>
> RelationReplicaIdentityFullIndexScanEnabled() is introduced
Hi Andres, Amit, all
I think the case in which the patch regresses performance in is irrelevant
> in
> practice.
>
This is similar to what I think in this context.
I appreciate the effort from Shi Yu, so that we have a clear understanding
on the overhead.
But the tests we do on [1] where we
Hi all,
Thanks for working on this.
> I imagine that a typical use case would be to set min_send_delay to
> several hours to days. I'm concerned that it could not be an
> acceptable trade-off for many users that the system cannot collect any
> garbage during that.
>
I'm not too worried about
Hi Amit, all
Amit Kapila , 6 Mar 2023 Pzt, 12:40 tarihinde şunu
yazdı:
> On Fri, Mar 3, 2023 at 6:40 PM Önder Kalacı wrote:
> >
> > Hi Vignesh,
> >
> > Thanks for the review
> >
> >>
> >> 1) We are currently calling RelationGetIndexList twice,
Hi Peter, all
>
> 1.
> A published table must have a replica identity configured in order to
> be able to replicate UPDATE and DELETE operations, so that appropriate
> rows to update or delete can be identified on the subscriber side. By
> default, this is the primary key, if there is one.
Hi Amit, all
>
> >
> > I think you've "simplified" this function in v28 but AFAICT now it has
> > a different logic to v27.
> >
> > PREVIOUSLY it was coded like
> > + return RelationGetReplicaIndex(rel) == idxoid ||
> > + RelationGetPrimaryKeyIndex(rel) == idxoid;
> >
> > You can see if 'idxoid'
Hi Amit, all
> >
> > Given Amit's suggestion on [1], I'm planning to drop this check
> altogether, and
> > rely on table storage parameters.
> >
>
> This still seems to be present in the latest version. I think we can
> just remove this and then add the additional check as suggested by you
> as
Hi Hayato, Amit, all
>
>
> ```
> + /* Build scankey for every non-expression attribute in the index.
> */
> ```
>
> I think that single line comments should not terminated by ".".
>
Hmm, looking into execReplication.c, many of the single line comments
terminated by ".". Also, On the
Hi Vignesh,
Thanks for the review
> 1) We are currently calling RelationGetIndexList twice, once in
> FindUsableIndexForReplicaIdentityFull function and in the caller too,
> we could avoid one of the calls by passing the indexlist to the
> function or removing the check here, index list check
Hi Amit, all
> Few comments on 0001
>
> 1.
> + such suitable indexes, the search on the subscriber s ide can be
> very inefficient,
>
> unnecessary space in 'side'
>
Fixed in v28
>
> 2.
> - identity. If the table does not have any suitable key, then it can be
> set
>
Hi Peter, all
>
> ==
> Commit Message
>
> 1.
> There is no smart mechanism to pick the index. Instead, we choose
> the first index that fulfils the requirements mentioned above.
>
> ~
>
> 1a.
> I think this paragraph should immediately follow the earlier one
> ("With this patch...") which
Hi Hou zj, all
>
> 1.
> + usableIndexContext = AllocSetContextCreate(CurrentMemoryContext,
> +
> "usableIndexContext",
> +
> ALLOCSET_DEFAULT_SIZES);
> + oldctx = MemoryContextSwitchTo(usableIndexContext);
> +
> + /* Get index list of the local
Hi Amit, Shi Yu
> >
> > b. Executed SQL.
> > I executed TRUNCATE and INSERT before each UPDATE. I am not sure if you
> did the
> > same, or just executed 50 consecutive UPDATEs. If the latter one, there
> would be
> > lots of old tuples and this might have a bigger impact on sequential
> scan. I
Hi Amit, all
Wouldn't a table-level option like 'apply_index_scan' be better than a
> subscription-level option with a default value as false? Anyway, the
> bigger point is that we don't see a better way to proceed here than to
> introduce some option to control this behavior.
>
What would be a
Hi Amit, all
Amit Kapila , 15 Şub 2023 Çar, 07:37 tarihinde
şunu yazdı:
> On Wed, Feb 15, 2023 at 9:23 AM shiy.f...@fujitsu.com
> wrote:
> >
> > On Sat, Feb 4, 2023 7:24 PM Amit Kapila wrote:
> > >
> > > On Thu, Feb 2, 2023 at 2:03 PM Önder Kalacı
> w
Hi all,
Thanks for the feedback!
I agree that it won't be a very convenient option for the user but how
> about along with asking for an index from the user (when the user
> didn't provide an index), we also allow to make use of any unique
> index over a subset of the transmitted columns,
Tbh,
Hi Marco, Tom,
> But still it doesn't seem to me to be appropriate to use the planner to
find a suitable index.
As Marco noted, here we are trying to pick an index that is non-unique. We
could pick the index based on information extracted from pg_index (or
such), but then, it'd be a premature
Hi,
Thank you for the useful comments!
> I took a very brief look through this. I'm not too pleased with
> this whole line of development TBH. It seems to me that the core
> design of execReplication.c and related code is "let's build our
> own half-baked executor and
Hi,
Thanks for the heads-up.
> Needs another rebase, I think:
>
> https://cirrus-ci.com/task/559244463758
>
> [05:44:22.102] FAILED:
> src/backend/postgres_lib.a.p/replication_logical_worker.c.o
> [05:44:22.102] ccache cc -Isrc/backend/postgres_lib.a.p -Isrc/include
> -I../src/include
more tests, I can do that. For reference, here are the tests that I did
manually: More Replication Index Tests (github.com)
<https://gist.github.com/onderkalaci/fa91688dea968e4024623feb4ddb627f>
Attached v21.
Onder KALACI
Önder Kalacı , 21 Eki 2022 Cum, 14:14 tarihinde şunu
yazdı:
>
Hi Hayota Kuroda,
> I (and my company) worried about overnight batch processing that
> contains some accesses to foreign servers. If the transaction is opened
> overnight and
> one of foreign servers is crashed during it, the transaction must be
> rollbacked.
> But there is a possibility that
Hi Shi yu, all
> In execReplication.c:
>
> + TypeCacheEntry **eq = NULL; /* only used when the index is not
> unique */
>
> Maybe the comment here should be changed. Now it is used when the index is
> not
> primary key or replica identity index.
>
>
makes sense, updated
> 2.
> +# wait
Hi,
> As far as I can think of, it should probably be a single background task
> > checking whether the server is down. If so, sending an invalidation
> message
> > to all the backends such that related backends could act on the
> > invalidation and throw an error. This is to cover the use-case
Hi Wang, all
> And I have another confusion about function
> GetCheapestReplicaIdentityFullPath:
> If rel->pathlist is NIL, could we return NULL directly from this function,
> and
> then set idxoid to InvalidOid in function
> FindUsableIndexForReplicaIdentityFull
> in that case?
>
>
We could,
Hi,
Thanks for the review!
Here are some comments on v17 patch.
>
> 1.
> -LogicalRepRelMapEntry *
> +LogicalRepPartMapEntry *
> logicalrep_partition_open(LogicalRepRelMapEntry *root,
> Relation partrel,
> AttrMap *map)
> {
>
> Is there any
Hi,
> ~~~
> 01. 032_subscribe_use_index.pl - SUBSCRIPTION CAN UPDATE THE INDEX IT
> USES AFTER ANALYZE
>
> ```
> # show that index_b is not used
> $node_subscriber->poll_query_until(
> 'postgres', q{select idx_scan=0 from pg_stat_all_indexes where
> indexrelname = 'index_b';}
> ) or
Hi,
Sounds reasonable. Do you mean that we can add additional GUC like
> "postgres_fdw.initial_check",
> wait WL_SOCKET_CLOSED if the conneciton is found in the hash table, and do
> reconnection if it might be closed, right?
>
>
Alright, it took me sometime to realize that postgres_fdw already
Hi Kuroda Hayato,
> ===
> 01. relation.c - GetCheapestReplicaIdentityFullPath
>
> ```
> * The reason that the planner would not pick partial indexes and
> indexes
> * with only expressions based on the way currently
> baserestrictinfos are
> * formed (e.g., col_1 = $1
Hi Kurado Hayato,
> In your patch:
>
> ```
> + /* Simple case, we already have a primary key or a replica
> identity index */
> + idxoid = GetRelationIdentityOrPK(localrel);
> + if (OidIsValid(idxoid))
> + return idxoid;
> +
> + /*
> +* If index
Hi Hayato Kuroda,
> If the checking function is called not periodically but GetConnection(),
> it means that the health of foreign servers will be check only when remote
> connections are used.
> So following workload described in [1] cannot handle the issue.
>
> BEGIN --- remote operations---
Hi Hayato Kuroda,
Thanks for the patch. I think there are some non-fdw extensions out there
which could benefit from this logic. That's why I want to first learn some
more about high-level design/goals of the patch a little more.
+/*
> + * Call callbacks for checking remote servers.
> + */
>
Hi David, Tom, all,
> I've just pushed the disable byref Datums patch I posted earlier. I
> only made a small adjustment to make use of the TupleDescAttr() macro.
> Önder, thank you for the report.
>
>
With this commit, I re-run the query patterns where we observed the
problem, all looks good
Hi Hayato Kuroda,
Thanks for the review!
>
> 01 relation.c - general
>
> Many files are newly included.
> I was not sure but some codes related with planner may be able to move to
> src/backend/optimizer/plan.
> How do you and any other one think?
>
>
My thinking on those functions is that
Hi,
Thanks for replying so quickly!
I run your test here with a fix attached.
>
> Can you retake your test with the patch attached?
>
>
> Unfortunately, with the patch, I still see the memory usage increase and
get the OOMs
Thanks,
Onder KALACI
Hi hackers,
With PG 15 (rc1 or beta4), I'm observing an interesting memory pattern. I
have not seen a similar discussion on the mailing list. If I missed that,
please refer me there. The problem that I'm going to explain does not
happen on PG 13/14.
It seems like there is a memory leak(?) with
Hii Wang wei,
>
> 1. In the function GetCheapestReplicaIdentityFullPath.
> + if (rel->pathlist == NIL)
> + {
> + /*
> +* A sequential scan could have been dominated by by an
> index scan
> +* during make_one_rel(). We should always have a
Hi Peter, Kuroda
kuroda.hay...@fujitsu.com , 21 Eyl 2022 Çar,
04:21 tarihinde şunu yazdı:
> > One last thing - do you think there is any need to mention this
> > behaviour in the pgdocs, or is OK just to be a hidden performance
> > improvement?
>
> FYI - I put my opinion.
> We have following
Hi Peter,
>
> 1. src/backend/executor/execReplication.c - build_replindex_scan_key
>
> - * This is not generic routine, it expects the idxrel to be replication
> - * identity of a rel and meet all limitations associated with that.
> + * This is not generic routine, it expects the idxrel to be
Hi Peter,
Thanks for the quick response.
> 1. Commit message
>
> It looks like some small mistake happened. You wrote [1] that my
> previous review comments about the commit message were fixed, but it
> seems the v11 commit message is unchanged since v10.
>
>
Oops, yes you are right, I forgot
Hi Peter,
Thanks again for the review, see my comments below:
>
> ==
>
> 1. Commit message
>
> It is often not feasible to use `REPLICA IDENTITY FULL` on the publication
> because it leads to full table scan per tuple change on the subscription.
> This makes `REPLICA IDENTITY FULL`
Hi Hayato Kuroda,
Thanks for the review, please see my reply below:
> ===
> For execRelation.c
>
> 01. RelationFindReplTupleByIndex()
>
> ```
> /* Start an index scan. */
> InitDirtySnapshot(snap);
> - scan = index_beginscan(rel, idxrel, ,
> -
>
Hi,
> >
> > Oh, I haven't considered inherited tables. That seems right, the
> statistics of the children are not updated when the parent is analyzed.
> >
> >>
> >> Now, the point I was worried about was what if the changes in child
> >> tables (*_part1, *_part2) are much more than in tbl1? In
Hi again,
> ==
>
> 1. Commit message
>
> 1a.
> With this patch, I'm proposing the following change: If there is an
> index on the subscriber, use the index as long as the planner
> sub-modules picks any index over sequential scan. The index should be
> a btree index, not a partital index.
Hi Peter,
Thanks for the reviews! I'll reply to both of your reviews separately.
> >> 10.
> >>
> >> + /* we only need to allocate once */
> >> + if (eq == NULL)
> >> + eq = palloc0(sizeof(*eq) * outslot->tts_tupleDescriptor->natts);
> >>
> >> But shouldn't you also free this 'eq' before the
Hi,
Thanks for the review!
>
> 1.
> In FilterOutNotSuitablePathsForReplIdentFull(), is
> "nonPartialIndexPathList" a
> good name for the list? Indexes on only expressions are also be filtered.
>
> +static List *
> +FilterOutNotSuitablePathsForReplIdentFull(List *pathlist)
> +{
> +
Hi,
I'm a little late to catch up with your comments, but here are my replies:
> My answer for the above assumes that your question is regarding what
> happens if you ANALYZE on a partitioned table. If your question is
> something different, please let me know.
> >
>
> I was talking about
Hi,
>
> FYI, I noticed that v5 causes cfbot failure in [1].
> Could you please fix it in the next version ?
>
Thanks for letting me know!
>
> [19:44:38.420] execReplication.c: In function
> ‘RelationFindReplTupleByIndex’:
> [19:44:38.420] execReplication.c:186:24: error: ‘eq’ may be used
>
Hi,
I should be able to grab a little bit of time next week to look at
> what you have.
>
Thanks!
>
> Please note that we should not add an event in create_am.sql even if
> it is empty, as it gets run in parallel of other tests so there could
> be interferences. I think that this had better
Hi,
Attached a patch to fix as well. If the patch looks good to you, can you
consider getting this to PG 15?
Steps to repro:
-- some basic examples from src/test/regress/sql/create_am.sql
CREATE TABLE heaptable USING heap AS
SELECT a, repeat(a::text, 100) FROM generate_series(1,9) AS a;
CREATE
Hi,
Thanks for the feedback, see my reply below.
>
> > It turns out that we already invalidate the relevant entries in
> LogicalRepRelMap/LogicalRepPartMap when "ANALYZE" (or VACUUM) updates any
> of the statistics in pg_class.
> >
> > The call-stack for analyze is roughly:
> > do_analyze_rel()
Hi,
As far as I can see, the following is the answer to the only remaining open
discussion in this thread. Let me know if anything is missed.
(b) it appears to me that the patch decides
> >> which index to use the first time it opens the rel (or if the rel gets
> >> invalidated) on subscriber
Hi,
2.
>> @@ -57,9 +60,6 @@ build_replindex_scan_key(ScanKey skey, Relation rel,
>> Relation idxrel,
>> int2vector *indkey = >rd_index->indkey;
>> bool hasnulls = false;
>>
>> - Assert(RelationGetReplicaIndex(rel) == RelationGetRelid(idxrel) ||
>> -RelationGetPrimaryKeyIndex(rel) ==
Hi,
>
> >
> > One another idea could be to re-calculate the index, say after N
> updates/deletes for the table. We may consider using subscription_parameter
> for getting N -- with a good default, or even hard-code into the code. I
> think the cost of re-calculating should really be pretty small
1 - 100 of 107 matches
Mail list logo
