Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
От | Cédric Villemain |
---|---|
Тема | Re: Extension Enhancement: Buffer Invalidation in pg_buffercache |
Дата | |
Msg-id | 8b8c709a-c7cc-4965-8296-64f549c27501@abcsql.com обсуждение исходный текст |
Ответ на | Re: Extension Enhancement: Buffer Invalidation in pg_buffercache (Palak Chaturvedi <chaturvedipalak1911@gmail.com>) |
Список | pgsql-hackers |
Hi Palak, I did a quick review of the patch: +CREATE FUNCTION pg_buffercache_invalidate(IN int, IN bool default true) +RETURNS bool +AS 'MODULE_PATHNAME', 'pg_buffercache_invalidate' +LANGUAGE C PARALLEL SAFE; --> Not enforced anywhere, but you can also add a comment to the function, for end users... +PG_FUNCTION_INFO_V1(pg_buffercache_invalidate); +Datum +pg_buffercache_invalidate(PG_FUNCTION_ARGS) +{ + Buffer bufnum; "Buffer blocknum" is not correct in this context I believe. Buffer is when you have to manage Local buffer too (negative number). Here uint32 is probably the good choice at the end, as used in pg_buffercache in other places. Also in this extension bufferid is used, not buffernum. + bufnum = PG_GETARG_INT32(0); + if (bufnum <= 0 || bufnum > NBuffers) maybe have a look at pageinspect and its PG_GETARG_UINT32. + { + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("buffernum is not valid"))); https://www.postgresql.org/docs/16/error-style-guide.html let me think that message like 'buffernum is not valid' can be enhanced: out of range, cannot be negative or exceed number of shared buffers.... ? Maybe add the value to the message. + + } + + /* + * Check whether to force invalidate the dirty buffer. The default value of force is true. + */ + + force = PG_GETARG_BOOL(1); I think you also need to test PG_ARGISNULL with force parameter. +/* + * Try Invalidating a buffer using bufnum. + * If the buffer is invalid, the function returns false. + * The function checks for dirty buffer and flushes the dirty buffer before invalidating. + * If the buffer is still dirty it returns false. + */ +bool +TryInvalidateBuffer(Buffer bufnum, bool force) +{ + BufferDesc *bufHdr = GetBufferDescriptor(bufnum - 1); this is not safe, GetBufferDescriptor() accepts uint, but can receive negative here. Use uint32 and bufferid. + uint32 buf_state; + ReservePrivateRefCountEntry(); + + buf_state = LockBufHdr(bufHdr); + if ((buf_state & BM_VALID) == BM_VALID) + { + /* + * The buffer is pinned therefore cannot invalidate. + */ + if (BUF_STATE_GET_REFCOUNT(buf_state) > 0) + { + UnlockBufHdr(bufHdr, buf_state); + return false; + } + if ((buf_state & BM_DIRTY) == BM_DIRTY) + { + /* + * If the buffer is dirty and the user has not asked to clear the dirty buffer return false. + * Otherwise clear the dirty buffer. + */ + if(!force){ + return false; probably need to unlockbuffer here too. + } + /* + * Try once to flush the dirty buffer. + */ + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); + PinBuffer_Locked(bufHdr); + LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED); + FlushBuffer(bufHdr, NULL, IOOBJECT_RELATION, IOCONTEXT_NORMAL); + LWLockRelease(BufferDescriptorGetContentLock(bufHdr)); + UnpinBuffer(bufHdr); I am unsure of this area (the code is correct, but I wonder why there is no static code for this part -from pin to unpin- in PostgreSQL), and maybe better to go with FlushOneBuffer() ? Also it is probably required to account for the shared buffer eviction in some pg_stat* view or table. Not sure how disk syncing is handled after this sequence nor if it's important ? + buf_state = LockBufHdr(bufHdr); + if (BUF_STATE_GET_REFCOUNT(buf_state) > 0) + { + UnlockBufHdr(bufHdr, buf_state); + return false; + } + + /* + * If its dirty again or not valid anymore give up. + */ + + if ((buf_state & (BM_DIRTY | BM_VALID)) != (BM_VALID)) + { + UnlockBufHdr(bufHdr, buf_state); + return false; + } + + } + + InvalidateBuffer(bufHdr); + return true; + } + else + { + UnlockBufHdr(bufHdr, buf_state); + return false; + } Maybe safe to remove the else {} ... Maybe more tempting to start the big if with the following instead less nested...): + if ((buf_state & BM_VALID) != BM_VALID) + { + UnlockBufHdr(bufHdr, buf_state); + return false; + } Doc and test are absent. --- Cédric Villemain +33 (0)6 20 30 22 52 https://Data-Bene.io PostgreSQL Expertise, Support, Training, R&D
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Jelte Fennema-NioДата:
Сообщение: Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs