Re: Make relfile tombstone files conditional on WAL level

2022-05-16 Thread Dilip Kumar
On Mon, May 16, 2022 at 3:24 PM Thomas Munro wrote: > > I think you can get rid of SYNC_UNLINK_REQUEST, sync_unlinkfiletag, > mdunlinkfiletag as these are all now unused. Correct. > Are there any special hazards here if the plan in [1] goes ahead? IMHO we should not have any problem. In fact,

Re: Make relfile tombstone files conditional on WAL level

2022-05-16 Thread Thomas Munro
I think you can get rid of SYNC_UNLINK_REQUEST, sync_unlinkfiletag, mdunlinkfiletag as these are all now unused. Are there any special hazards here if the plan in [1] goes ahead? If the relfilenode allocation is logged and replayed then it should be fine to crash and recover multiple times in a

Re: Make relfile tombstone files conditional on WAL level

2022-05-15 Thread Dilip Kumar
On Thu, May 12, 2022 at 4:27 PM Amul Sul wrote: > Hi Amul, Thanks for the review, actually based on some comments from Robert we have planned to make some design changes. So I am planning to work on that for the July commitfest. I will try to incorporate all your review comments in the new

Re: Make relfile tombstone files conditional on WAL level

2022-05-12 Thread Amul Sul
Hi Dilip, On Fri, Mar 4, 2022 at 11:07 AM Dilip Kumar wrote: > > On Mon, Feb 21, 2022 at 1:21 PM Dilip Kumar wrote: > > > > On Thu, Jan 6, 2022 at 1:43 PM Dilip Kumar wrote: > > > > 2) GetNewRelFileNode() will not loop for checking the file existence > > > and retry with other relfilenode. >

Re: Make relfile tombstone files conditional on WAL level

2022-03-08 Thread Robert Haas
On Fri, Mar 4, 2022 at 12:37 AM Dilip Kumar wrote: > In this version I have fixed both of these issues. Here's a bit of review for these patches: - The whole relnode vs. relfilenode thing is really confusing. I realize that there is some precedent for calling the number that pertains to the

Re: Make relfile tombstone files conditional on WAL level

2022-03-07 Thread Dilip Kumar
On Tue, Mar 8, 2022 at 1:27 AM Robert Haas wrote: > > The only part I do not like in the patch is that before this patch we > > could directly access the buftag->rnode. But since now we are not > > having directly relfilenode as part of the buffertag and instead of > > that we are keeping

Re: Make relfile tombstone files conditional on WAL level

2022-03-07 Thread Robert Haas
On Fri, Mar 4, 2022 at 12:37 AM Dilip Kumar wrote: > In this version I have fixed both of these issues. Thanks Robert for > suggesting the solution for both of these problems in our offlist > discussion. Basically, for the first problem we can flush the xlog > immediately because we are

Re: Make relfile tombstone files conditional on WAL level

2022-03-07 Thread Robert Haas
On Mon, Feb 21, 2022 at 2:51 AM Dilip Kumar wrote: > While working on this I realized that even if we make the relfilenode > 56 bits we can not remove the loop inside GetNewRelFileNode() for > checking the file existence. Because it is always possible that the > file reaches to the disk even

Re: Make relfile tombstone files conditional on WAL level

2022-02-09 Thread Dilip Kumar
On Mon, Feb 7, 2022 at 10:13 PM Robert Haas wrote: > > On Mon, Feb 7, 2022 at 11:31 AM Dilip Kumar wrote: > > For RelFileNode also we need to use 2, 32-bit integers so that we do > > not add extra alignment padding because there are a few more > > structures that include RelFileNode e.g.

Re: Make relfile tombstone files conditional on WAL level

2022-02-07 Thread Robert Haas
On Mon, Feb 7, 2022 at 11:31 AM Dilip Kumar wrote: > For RelFileNode also we need to use 2, 32-bit integers so that we do > not add extra alignment padding because there are a few more > structures that include RelFileNode e.g. xl_xact_relfilenodes, > RelFileNodeBackend, and many other

Re: Make relfile tombstone files conditional on WAL level

2022-02-07 Thread Dilip Kumar
On Mon, Feb 7, 2022 at 9:42 PM Robert Haas wrote: > > On Mon, Feb 7, 2022 at 12:26 AM Dilip Kumar wrote: > > I have splitted the patch into multiple patches which can be > > independently committable and easy to review. I have explained the > > purpose and scope of each patch in the respective

Re: Make relfile tombstone files conditional on WAL level

2022-02-07 Thread Robert Haas
On Mon, Feb 7, 2022 at 12:26 AM Dilip Kumar wrote: > I have splitted the patch into multiple patches which can be > independently committable and easy to review. I have explained the > purpose and scope of each patch in the respective commit messages. Hmm. The parts of this I've looked at seem

Re: Make relfile tombstone files conditional on WAL level

2022-02-02 Thread Dilip Kumar
On Wed, Feb 2, 2022 at 6:57 PM Robert Haas wrote: > > On Mon, Jan 31, 2022 at 9:37 AM Dilip Kumar wrote: > > I agree that we are using 8 bytes unsigned int multiple places in code > > as uint64. But I don't see it as an exposed data type and not used as > > part of any exposed function. But we

Re: Make relfile tombstone files conditional on WAL level

2022-02-02 Thread Robert Haas
On Mon, Jan 31, 2022 at 9:37 AM Dilip Kumar wrote: > I agree that we are using 8 bytes unsigned int multiple places in code > as uint64. But I don't see it as an exposed data type and not used as > part of any exposed function. But we will have to use the relfilenode > in the exposed c function

Re: Make relfile tombstone files conditional on WAL level

2022-01-31 Thread Dilip Kumar
On Mon, Jan 31, 2022 at 7:36 PM Robert Haas wrote: > > On Mon, Jan 31, 2022 at 9:04 AM Robert Haas wrote: > > On Mon, Jan 31, 2022 at 12:29 AM Dilip Kumar wrote: > > > the main one currently we do not have uint8 data > > > type only int8 is there so I have used int8 for storing relfilenode + >

Re: Make relfile tombstone files conditional on WAL level

2022-01-31 Thread Robert Haas
On Mon, Jan 31, 2022 at 9:04 AM Robert Haas wrote: > On Mon, Jan 31, 2022 at 12:29 AM Dilip Kumar wrote: > > the main one currently we do not have uint8 data > > type only int8 is there so I have used int8 for storing relfilenode + > > forknumber. > > I'm confused. We use int8 in tons of places,

Re: Make relfile tombstone files conditional on WAL level

2022-01-31 Thread Robert Haas
On Mon, Jan 31, 2022 at 12:29 AM Dilip Kumar wrote: > the main one currently we do not have uint8 data > type only int8 is there so I have used int8 for storing relfilenode + > forknumber. I'm confused. We use int8 in tons of places, so I feel like it must exist. -- Robert Haas EDB:

Re: Make relfile tombstone files conditional on WAL level

2022-01-28 Thread Dilip Kumar
On Wed, Jan 19, 2022 at 10:37 AM Dilip Kumar wrote: > > On Thu, Jan 6, 2022 at 7:22 PM Robert Haas wrote: >> >> On Thu, Jan 6, 2022 at 3:47 AM Thomas Munro wrote: >> > Another problem is that relfilenodes are normally allocated with >> > GetNewOidWithIndex(), and initially match a relation's

Re: Make relfile tombstone files conditional on WAL level

2022-01-18 Thread Dilip Kumar
On Thu, Jan 6, 2022 at 7:22 PM Robert Haas wrote: > On Thu, Jan 6, 2022 at 3:47 AM Thomas Munro > wrote: > > Another problem is that relfilenodes are normally allocated with > > GetNewOidWithIndex(), and initially match a relation's OID. We'd need > > a new allocator, and they won't be able to

Re: Make relfile tombstone files conditional on WAL level

2022-01-06 Thread Andres Freund
On 2022-01-06 08:52:01 -0500, Robert Haas wrote: > On Thu, Jan 6, 2022 at 3:47 AM Thomas Munro wrote: > > Another problem is that relfilenodes are normally allocated with > > GetNewOidWithIndex(), and initially match a relation's OID. We'd need > > a new allocator, and they won't be able to

Re: Make relfile tombstone files conditional on WAL level

2022-01-06 Thread Robert Haas
On Thu, Jan 6, 2022 at 3:47 AM Thomas Munro wrote: > Another problem is that relfilenodes are normally allocated with > GetNewOidWithIndex(), and initially match a relation's OID. We'd need > a new allocator, and they won't be able to match the OID in general > (while we have 32 bit OIDs at

Re: Make relfile tombstone files conditional on WAL level

2022-01-06 Thread Thomas Munro
On Thu, Jan 6, 2022 at 9:13 PM Dilip Kumar wrote: > On Thu, Jan 6, 2022 at 1:12 PM Dilip Kumar wrote: > > > I think this idea is worth more consideration. It seems like 2^56 > > > relfilenodes ought to be enough for anyone, recalling that you can > > > only ever have 2^64 bytes of WAL. So if we

Re: Make relfile tombstone files conditional on WAL level

2022-01-06 Thread Dilip Kumar
On Thu, Jan 6, 2022 at 1:12 PM Dilip Kumar wrote: > > > > > I think this idea is worth more consideration. It seems like 2^56 > > relfilenodes ought to be enough for anyone, recalling that you can > > only ever have 2^64 bytes of WAL. So if we do this, we can eliminate a > > bunch of code that is

Re: Make relfile tombstone files conditional on WAL level

2022-01-05 Thread Dilip Kumar
On Thu, Jan 6, 2022 at 3:07 AM Robert Haas wrote: > > On Mon, Aug 2, 2021 at 6:38 PM Andres Freund wrote: > > I guess there's a somewhat hacky way to get somewhere without actually > > increasing the size. We could take 3 bytes from the fork number and use that > > to get to a 7 byte relfilenode

Re: Make relfile tombstone files conditional on WAL level

2022-01-05 Thread Robert Haas
On Mon, Aug 2, 2021 at 6:38 PM Andres Freund wrote: > I guess there's a somewhat hacky way to get somewhere without actually > increasing the size. We could take 3 bytes from the fork number and use that > to get to a 7 byte relfilenode portion. 7 bytes are probably enough for > everyone. > >

Re: Make relfile tombstone files conditional on WAL level

2021-10-28 Thread Thomas Munro
On Tue, Oct 5, 2021 at 4:21 PM Thomas Munro wrote: > On Thu, Sep 30, 2021 at 11:32 PM Thomas Munro wrote: > > I managed to produce a case where live data is written to an unlinked > > file and lost In conclusion, there *is* something else that would break, so I'm withdrawing this CF entry

Re: Make relfile tombstone files conditional on WAL level

2021-10-04 Thread Thomas Munro
On Thu, Sep 30, 2021 at 11:32 PM Thomas Munro wrote: > I managed to produce a case where live data is written to an unlinked > file and lost I guess this must have been broken since release 9.2 moved checkpoints out of here[1]. The connection between checkpoints, tombstone files and file

Re: Make relfile tombstone files conditional on WAL level

2021-09-30 Thread Thomas Munro
On Wed, Sep 29, 2021 at 4:07 PM Thomas Munro wrote: > Hmm, on closer inspection, isn't the lack of real interlocking with > checkpoints a bit suspect already? What stops bgwriter from writing > to the previous relfilenode generation's fd, if a relfilenode is > recycled while BgBufferSync() is

Re: Make relfile tombstone files conditional on WAL level

2021-09-28 Thread Thomas Munro
On Wed, Aug 4, 2021 at 3:22 AM Robert Haas wrote: > It's not really clear to me what problem is at hand. The problems that > the tombstone system created for the async I/O stuff weren't really > explained properly, IMHO. And I don't think the current system is all > that ugly. it's not the most

Re: Make relfile tombstone files conditional on WAL level

2021-09-28 Thread Thomas Munro
On Fri, Mar 5, 2021 at 11:02 AM Thomas Munro wrote: > This is a WIP with an open question to research: what could actually > break if we did this? I thought this part of bgwriter.c might be a candidate: if (FirstCallSinceLastCheckpoint()) { /* * After any

Re: Make relfile tombstone files conditional on WAL level

2021-08-03 Thread Robert Haas
On Mon, Aug 2, 2021 at 6:38 PM Andres Freund wrote: > What I proposed in the past was to have a new shared table that tracks > relfilenodes. I still think that's a decent solution for just the problem at > hand. It's not really clear to me what problem is at hand. The problems that the tombstone

Re: Make relfile tombstone files conditional on WAL level

2021-08-02 Thread Andres Freund
Hi, On 2021-08-02 16:03:31 -0400, Robert Haas wrote: > The two most principled solutions to this problem that I can see are > (1) remove wal_level=minimal and I'm personally not opposed to this. It's not practically relevant and makes a lot of stuff more complicated. We imo should rather focus

Re: Make relfile tombstone files conditional on WAL level

2021-08-02 Thread Robert Haas
On Thu, Jun 10, 2021 at 6:47 AM Heikki Linnakangas wrote: > It would indeed be nice to have some other mechanism to prevent the > issue with wal_level=minimal, the tombstone files feel hacky and > complicated. Maybe a new shared memory hash table to track the > relfilenodes of dropped tables.

Re: Make relfile tombstone files conditional on WAL level

2021-06-10 Thread Heikki Linnakangas
On 05/03/2021 00:02, Thomas Munro wrote: Hi, I'm starting a new thread for this patch that originated as a side-discussion in [1], to give it its own CF entry in the next cycle. This is a WIP with an open question to research: what could actually break if we did this? I don't see a problem.

Make relfile tombstone files conditional on WAL level

2021-03-04 Thread Thomas Munro
%2BhUKGLdemy2gBm80kz20GTe6hNVwoErE8KwcJk6-U56oStjtg%40mail.gmail.com From 61a15ed286a1fd824b4e2b4b689cbe6688930e6e Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Tue, 2 Mar 2021 16:09:51 +1300 Subject: [PATCH] Make relfile tombstone files conditional on WAL level. Traditionally we have left behind an empty file