Re: SLRUs in the main buffer pool, redux

2023-01-20 Thread Shawn Debnath
On Mon, Jul 25, 2022 at 11:54:36AM +0300, Heikki Linnakangas wrote:

> Oh I just saw that you had a comment about that in the patch and had hacked
> around it. Anyway, calling ResourceOwnerEnlargeBuffers() might be a
> solution. Or switch to a separate "CriticalResourceOwner" that's guaranteed
> to have enough pre-allocated space, before entering the critical section.

Wanted to bump up this thread. Rishu in my team posted a patch in the other 
SLRU thread [1] with the latest updates and fixes and looks like performance 
numbers do not show any regression. This change is currently in the 
January commitfest [2] as well. Any feedback would be appreciated!

[1]
https://www.postgresql.org/message-id/A09EAE0D-0D3F-4A34-ADE9-8AC1DCBE7D57%40amazon.com
[2] https://commitfest.postgresql.org/41/3514/

Shawn 
Amazon Web Services (AWS)




Re: SLRUs in the main buffer pool - Page Header definitions

2022-06-24 Thread Shawn Debnath
On Thu, Jun 23, 2022 at 06:06:48PM -0700, Andres Freund wrote:

> > You are correct that we wouldn’t need to rely on the pd_flag bit to
> > determine page type for any access to a page where we come top down
> > following the hierarchy. However, for the purpose of debugging “from the
> > bottom up” it would be critical to know what type of page is being read in a
> > system with multiple page header types.
> 
> That doesn't seem to justify using a bit on the page. Wouldn't it suffice to
> add such information to the BufferDesc?

The goal for the bit in pd_flags is to help identify what page header 
should be present if one were to be looking at the physical page outside 
of the database. One example that comes to mind is pg_upgrade.  There 
are other use cases where physical consistency checks could be applied, 
again outside of a running database.

On Thu, Jun 23, 2022 at 04:27:33PM -0400, Robert Haas wrote:

> I think that it's not worth introducing a new page header format to
> save 10 bytes per page. Keeping things on the same format is likely to
> save more than the minor waste of space costs.

Yeah, I think we are open to both approaches, though we believe it would 
be cleaner to get started with a targeted page header for the new code.  
But do understand having to understand/translate/deal with two page 
header types might not be worth the savings in space.

If we stick with the current page header, of course, changes to pd_flag 
won't be necessary anymore.

Stepping back, it seems like folks are okay with introducing a page 
header to current SLRU components and that we are leaning towards 
sticking with the default one for now. We can proceed with this 
approach, and if needed, change it later if more folks chime in.

Cheers.

-- 
Shawn Debnath
Amazon Web Services (AWS)




Re: SLRUs in the main buffer pool - Page Header definitions

2022-06-30 Thread Shawn Debnath
On Fri, Jun 24, 2022 at 03:45:34PM -0700, Andres Freund wrote:

> Outside the database you'll know the path to the file, which will tell you
> it's not another kind of relation.
> 
> This really makes no sense to me. We don't have page flags indicating whether
> a page is a heap, btree, visibility, fms whatever kind of page either. On a
> green field, it'd make sense to have such information in a metapage at the
> start of each relation - but not on each page.

Alright, I agree with the metapage having the necessary info. In
this case, we can rely on the hierarchy to determine the type of header.
Given we do not have an usecase requiring the flag, we should not
consume it at this point.


> > On Thu, Jun 23, 2022 at 04:27:33PM -0400, Robert Haas wrote:
> >
> > > I think that it's not worth introducing a new page header format to
> > > save 10 bytes per page. Keeping things on the same format is likely to
> > > save more than the minor waste of space costs.
> >
> > Yeah, I think we are open to both approaches, though we believe it would
> > be cleaner to get started with a targeted page header for the new code.
> > But do understand having to understand/translate/deal with two page
> > header types might not be worth the savings in space.
> 
> Not sure if that changes anything, but it's maybe worth noting that we already
> have some types of pages that don't use the full page header (at least
> freespace.c, c.f. buffer_std argument to MarkBufferDirtyHint()). I can see an
> argument for shrinking the "uniform" part of the page header, and pushing more
> things down into AMs. But I think that'd need to change the existing code, not
> just introduce something new for new code.

We did think through a universal page header concept that included just
the pd_lsn, pd_checksum, pd_flags and pulling in pd_pagesize_version and other
fields as the non-uniform members for SLRU. Unfortunately, there is a gap of
48 bits after pd_flags which makes it challenging with today's header.  I am
leaning towards the standard page header for now and revisiting the 
universal/uniform
page header and AM changes in a separate effort. The push down to AM is an
interesting concept and should be worthwhile following up on.


> > Stepping back, it seems like folks are okay with introducing a page
> > header to current SLRU components and that we are leaning towards
> > sticking with the default one for now. We can proceed with this
> > approach, and if needed, change it later if more folks chime in.
> 
> I think we're clearly going to have to do that at some point not too far
> away. There's just too many capabilities that are made harder by not having
> that information for SLRU pages. That's not to say that it's a prerequisite to
> moving SLRUs into the buffer pool (using a hack like Thomas did until the page
> header is introduced). Both are complicated enough projects on their own. I
> also could see adding the page header before moving SLRUs in the buffer pool,
> there isn't really a hard dependency.

To be honest, given the nature of changes, I would prefer to have it
done in one major version release than have it be split into multiple
efforts. The value add really comes in from the consistency checks that
can be done and which are non-existent for SLRU today.