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 HEAD, the same comment has single line comment. So, I'd rather stick to that? > > ``` > + /* There should always be at least one attribute for the index > scan. */ > ``` > > Same as above. > Same as above :) > > > ``` > +#ifdef USE_ASSERT_CHECKING > + IndexInfo *indexInfo = BuildIndexInfo(idxrel); > + > + Assert(!IsIndexOnlyOnExpression(indexInfo)); > +#endif > ``` > > I may misunderstand, but the condition of usable index has alteady been > checked > when the oid was set but anyway the you confirmed the condition again > before > really using that, right? > So is it OK not to check another assumption that the index is b-tree, > non-partial, > and one column reference? IIUC we can do that by adding new function like > IsIndexUsableForReplicaIdentityFull() > that checks these condition, and then call at > RelationFindReplTupleByIndex() if > idxIsRelationIdentityOrPK is false. > I think adding a function like IsIndexUsableForReplicaIdentityFull is useful. I can use it inside FindUsableIndexForReplicaIdentityFull() and assert here. Also good for readability. So, I mainly moved this assert to a more generic place with a more generic check to RelationFindReplTupleByIndex > > 032_subscribe_use_index.pl > > ``` > +# Testcase start: SUBSCRIPTION CREATE/DROP INDEX WORKS WITHOUT ISSUES > ... > +# Testcase end: SUBSCRIPTION RE-CALCULATES INDEX AFTER CREATE/DROP INDEX > ``` > > There is still non-consistent case. > > Fixed, thanks Anyway, I see your point, so > if you want to keep the naming as you proposed at least don't change > the ordering for get_opclass_input_type() call because that looks odd > to me. (A small comment from Amit's previous e-mail) Sure, applied now. Attaching v31. Thanks for the reviews! Onder KALACI
v31_0001_use_index_on_subs_when_pub_rep_ident_full.patch
Description: Binary data
