Hi Alexandre,
Thanks for the patch. I read v1, here are my suggestions.
+ if (msg->id == SHAREDINVALSMGR_ID)
+ {
+ RelFileLocatorBackend rlocator;
+ rlocator.locator = msg->sm.rlocator;
+ rlocator.backend = (msg->sm.backend_hi << 16) | (int) msg->sm.backend_lo;
+ smgrreleaserellocator(rlocator);
+ }
Since 21d9c3ee4ef, smgrreleaserellocator() only closes the fds. The SMgrRelation
entry stays in the hash table. Backends free those in AtEOXact_SMgr(), but an
IO worker never runs a transaction, so entries for dropped relations
pile up. The fd leak turns into a memory leak. We had runs into production
issue because of that.
I see now, what I understood is that smgrrelease() only closes, smgrdestroy() closes
and releases memory. Updated the patch and the function comments trying to make this
distinction more evident.
bgwriter and checkpointer have the same problem. They call
smgrdestroyall() once per checkpoint cycle, see the comment in
bgwriter.c. Could the IO worker just do the same every so often?
I saw a proposal for what you described [1], but it doesn't seem ideal as
It keeps stale handles for much longer than necessary, and closes files
unnecessarily after each checkpoint.
That
is option 1 in the bug report. It needs no sinval slot for up to 32
workers,
Are the sinval entries concerning? If so, I think it could have something way simpler,
since we have a reset for when the buffer overflows, all we would need to have is
a circular queue of messages and each worker saving its position. If pending messages
is above the circular queue capacity it is a reset. This way the invalidation list doesn't
even need to be aware of who is reading it.
and it is small enough to backpatch to 18, where
io_method=worker is the default.
Indeed that is smaller, maybe backpatch with [1] and using this going forward.
The patch also needs a pgindent run.
Applied this time.
While checking SharedInvalBackendInit I noticed this
sinval.c
287 /*
288 * This can run in parallel with read operations, but not with write
289 * operations, since SIInsertDataEntries relies on the pgprocnos array to
290 * set hasMessages appropriately.
291 */
292 LWLockAcquire(SInvalWriteLock, LW_EXCLUSIVE);
It sounds like we could use a LW_SHARED lock there.
Regards,
Alexandre
