Re: More tests with USING INDEX replident and dropped indexes

2020-09-04 Thread Michael Paquier
On Wed, Sep 02, 2020 at 03:00:44PM +0900, Michael Paquier wrote: > Yeah, that's true as well. Still, I would like to see first if people > are fine with changing this code path to be transactional. This way, > we will have zero history in the tree where there was a risk for an > inconsistent wind

Re: More tests with USING INDEX replident and dropped indexes

2020-09-02 Thread Rahila Syed
> > > > On the other hand, it looks appealing to make index_set_state_flags() > transactional. This would solve the consistency problem, and looking > at the code scanning pg_index, I don't see a reason why we could not > do that. What do you think? > TBH, I am not sure. I think it is a reasona

Re: More tests with USING INDEX replident and dropped indexes

2020-09-01 Thread Michael Paquier
On Wed, Sep 02, 2020 at 09:27:33AM +0530, Rahila Syed wrote: > TBH, I am not sure. I think it is a reasonable change. It is even > indicated in the > comment above index_set_state_flags() that it can be made transactional. > At the same time, probably we can just go ahead with current > inconsiste

Re: More tests with USING INDEX replident and dropped indexes

2020-08-31 Thread Rahila
Hi Michael, Thanks for updating the patch. Please see following comments, 1.     */ if (resetreplident) {     SetRelationReplIdent(heapId, REPLICA_IDENTITY_NOTHING);     /* make the update visible, only for the non-concurrent case */    

Re: More tests with USING INDEX replident and dropped indexes

2020-08-30 Thread Michael Paquier
On Mon, Aug 31, 2020 at 10:36:13AM +0530, Rahila wrote: > Now, the relreplident is being set in the transaction previous to > the one marking index as invalid for concurrent drop. Won't > it cause issues with relreplident cleared but index not dropped, > if drop index concurrently fails in the smal

Re: More tests with USING INDEX replident and dropped indexes

2020-08-30 Thread Michael Paquier
On Thu, Aug 27, 2020 at 11:28:35AM +0900, Michael Paquier wrote: > Attached is a patch for 1) and 2) grouped together, to ease review for > now. I think that we had better fix 1) separately though, so I am > going to start a new thread about that with a separate patch as the > current thread is mi

Re: More tests with USING INDEX replident and dropped indexes

2020-08-26 Thread Michael Paquier
On Wed, Aug 26, 2020 at 09:08:51PM +0900, Michael Paquier wrote: > I'll go through this part again tomorrow, it is late here. There is actually a huge gotcha here that exists since replica identities exist: relation_mark_replica_identity() only considers valid indexes! So, on HEAD, if DROP INDEX

Re: More tests with USING INDEX replident and dropped indexes

2020-08-26 Thread Michael Paquier
On Tue, Aug 25, 2020 at 08:59:37PM +0530, Rahila wrote: > Sorry, I didn't apply correctly. The tests pass for me. In addition, I > tested with partitioned tables. It works as expected and makes the REPLICA > IDENTITY 'n' for the partitions as well when an index on a partitioned > table is dropped

Re: More tests with USING INDEX replident and dropped indexes

2020-08-25 Thread Rahila
Hi, I couldn't test the patch as it does not apply cleanly on master. The CF bot is green, and I can apply v2 cleanly FWIW: http://commitfest.cputube.org/michael-paquier.html Sorry, I didn't apply correctly.  The  tests pass for me. In addition, I tested with partitioned tables.  It works

Re: More tests with USING INDEX replident and dropped indexes

2020-08-19 Thread Michael Paquier
On Wed, Aug 19, 2020 at 05:33:36PM +0530, Rahila Syed wrote: > I couldn't test the patch as it does not apply cleanly on master. The CF bot is green, and I can apply v2 cleanly FWIW: http://commitfest.cputube.org/michael-paquier.html > Please find below some review comments: > > 1. Would it bett

Re: More tests with USING INDEX replident and dropped indexes

2020-08-19 Thread Rahila Syed
Hi, I couldn't test the patch as it does not apply cleanly on master. Please find below some review comments: 1. Would it better to throw a warning at the time of dropping the REPLICA IDENTITY index that it would also  drop the REPLICA IDENTITY of the parent table? 2. CCI is used after cal

Re: More tests with USING INDEX replident and dropped indexes

2020-08-16 Thread Michael Paquier
On Wed, Jun 03, 2020 at 12:08:56PM -0300, Euler Taveira wrote: > Consistency is a good goal. Why don't we clear the relreplident from the > relation while dropping the index? relation_mark_replica_identity() already > does that but do other things too. Let's move the first code block from > relatio

Re: More tests with USING INDEX replident and dropped indexes

2020-06-03 Thread Euler Taveira
On Wed, 3 Jun 2020 at 03:14, Michael Paquier wrote: > On Tue, Jun 02, 2020 at 04:46:55PM +0900, Masahiko Sawada wrote: > > How about avoiding such an inconsistent situation? In that case, > > replica identity works as NOTHING, but pg_class.relreplident is still > > ‘i’, confusing users. It seems

Re: More tests with USING INDEX replident and dropped indexes

2020-06-02 Thread Michael Paquier
On Tue, Jun 02, 2020 at 04:46:55PM +0900, Masahiko Sawada wrote: > How about avoiding such an inconsistent situation? In that case, > replica identity works as NOTHING, but pg_class.relreplident is still > ‘i’, confusing users. It seems to me that dropping an index specified > by REPLICA IDENTITY U

Re: More tests with USING INDEX replident and dropped indexes

2020-06-02 Thread Masahiko Sawada
On Fri, 22 May 2020 at 12:50, Michael Paquier wrote: > > Hi all, > > While working on some other logical decoding patch recently, I bumped > into the fact that we have special handling for the case of REPLICA > IDENTITY USING INDEX when the dependent index is dropped, where the > code handles that

More tests with USING INDEX replident and dropped indexes

2020-05-21 Thread Michael Paquier
Hi all, While working on some other logical decoding patch recently, I bumped into the fact that we have special handling for the case of REPLICA IDENTITY USING INDEX when the dependent index is dropped, where the code handles that case as an equivalent of NOTHING. Attached is a patch to add more