Thread: postgres_fdw: misplaced? comments in connection.c

Hi,

The comments for pgfdw_get_cleanup_result() say this:

 * It's not a huge problem if we throw an ERROR here, but if we get into error
 * recursion trouble, we'll end up slamming the connection shut, which will
 * necessitate failing the entire toplevel transaction even if subtransactions
 * were used.  Try to use WARNING where we can.

But we don’t use WARNING anywhere in that function.  The right place
for this is pgfdw_exec_cleanup_query()?

Best regards,
Etsuro Fujita

On Mon, Oct 11, 2021 at 5:05 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> The comments for pgfdw_get_cleanup_result() say this:
>
>  * It's not a huge problem if we throw an ERROR here, but if we get into error
>  * recursion trouble, we'll end up slamming the connection shut, which will
>  * necessitate failing the entire toplevel transaction even if subtransactions
>  * were used.  Try to use WARNING where we can.
>
> But we don’t use WARNING anywhere in that function.  The right place
> for this is pgfdw_exec_cleanup_query()?

I noticed that pgfdw_cancel_query(), which is called during (sub)abort
cleanup if necessary, also uses WARNING, instead of ERROR, to avoid
the error-recursion-trouble issue.  So I think it would be good to
move this to pgfdw_cancel_query() as well as
pgfdw_exec_cleanup_query().  Attached is a patch for that.

Best regards,
Etsuro Fujita

On Tue, Oct 12, 2021 at 1:33 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Mon, Oct 11, 2021 at 5:05 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > The comments for pgfdw_get_cleanup_result() say this:
> >
> >  * It's not a huge problem if we throw an ERROR here, but if we get into error
> >  * recursion trouble, we'll end up slamming the connection shut, which will
> >  * necessitate failing the entire toplevel transaction even if subtransactions
> >  * were used.  Try to use WARNING where we can.
> >
> > But we don’t use WARNING anywhere in that function.  The right place
> > for this is pgfdw_exec_cleanup_query()?
>
> I noticed that pgfdw_cancel_query(), which is called during (sub)abort
> cleanup if necessary, also uses WARNING, instead of ERROR, to avoid
> the error-recursion-trouble issue.  So I think it would be good to
> move this to pgfdw_cancel_query() as well as
> pgfdw_exec_cleanup_query().  Attached is a patch for that.

There seems to be no objections, so I have applied the patch.

Best regards,
Etsuro Fujita