Re: logical decoding : exceeded maxAllocatedDescs for .spill files

On Thu, Dec 5, 2019 at 4:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Dec 3, 2019 at 11:10 AM Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
> >
> >
> > Done as stated above; attached v3 patch. I have verified that the file
> > handles do get closed in PG_CATCH block via
> > ReorderBufferIterTXNFinish().
> >
>
> I couldn't reproduce the original problem (on HEAD) reported with the
> test case in the patch. So, I can't verify the fix. I think it is
> because of recent commits cec2edfa7859279f36d2374770ca920c59c73dd8 and
> 9290ad198b15d6b986b855d2a58d087a54777e87. It seems you need to either
> change the value of logical_decoding_work_mem or change the test in
> some way so that the original problem can be reproduced.
>

Few comments:
----------------------

1.
+ /* Now that the state fields are initialized, it is safe to return it. */
+ *iter_state = state;
+
/* allocate heap */
state->heap =
binaryheap_allocate(state->nr_txns,
ReorderBufferIterCompare,

Is there a reason for not initializing iter_state after
binaryheap_allocate? If we do so, then we don't need additional check
you have added in ReorderBufferIterTXNFinish.

2.
/* No harm in resetting the offset even in case of failure */
file->curOffset = 0;

The above comment is not clear because you are not setting it in case
of error rather this is a success path.

3.
+ *
+ * Note: The iterator state is returned through iter_state parameter rather
+ * than the function's return value. This is because the state gets cleaned up
+ * in a PG_CATCH block, so we want to make sure the caller gets back the state
+ * even if this function throws an exception, so that the state resources can
+ * be cleaned up.

How about changing it slightly as follows to make it more clear.
"Note: The iterator state is returned through iter_state parameter
rather than the function's return value. This is because the state
gets cleaned up in a PG_CATCH block in the caller, so we want to make
sure the caller gets back the state even if this function throws an
exception."

4. I think we should also check how much time increase will happen for
test_decoding regression test after the test added by this patch?

5. One naive question about the usage of PathNameOpenFile(). When it
reaches the max limit, it will automatically close one of the files,
but how will that be reflected in the data structure (TXNEntryFile)
you are managing. Basically, when PathNameOpenFile closes some file,
how will the corresponding vfd in TXNEntryFile be changed. Because if
it is not changed, then won't it start pointing to some wrong
filehandle?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com