Обсуждение: BGWORKER_BYPASS_ALLOWCONN used nowhere (infra part of on-linechecksum switcher)

Hi all,

BGWORKER_BYPASS_ALLOWCONN has been added by commit eed1ce7, which is an
infrastructure piece to be able to enable and disable dynamically
checksums on a cluster.  The main idea is to be able to bypass
datallowconn which allows a background worker to connect to a database
even if the database is set to refuse connections so as its checksums
can be calculated and updated.

At the end, the dynamic switch for checksums has been reverted as of
a228cc13, and a set of rather-used APIs have been changed for what looks
like no reason now:
- BackgroundWorkerInitializeConnection
- BackgroundWorkerInitializeConnectionByOid
- InitPostgres
So all background workers would not be able to compile because of that.
Would we want to drop this unused interface or keep it?

Even if this is not removed, bgworker.sgml needs to be updated with the
new definition of BackgroundWorkerInitializeConnection and
BackgroundWorkerInitializeConnectionByOid which are missing the third
argument "uint32 flags", as well as the description for
BGWORKER_BYPASS_ALLOWCONN.  I can personally see more reasons to revert
that portion as well and consider it again for v12 or onwards if the
on-line checksum switch is proposed again.

Magnus, Daniel, what do you think?

Thanks,
--
Michael

> On 22 Apr 2018, at 14:04, Magnus Hagander <magnus@hagander.net> wrote:
>
> On Sun, Apr 22, 2018 at 1:11 PM, Michael Paquier <michael@paquier.xyz <mailto:michael@paquier.xyz>> wrote:
> Hi all,
>
> BGWORKER_BYPASS_ALLOWCONN has been added by commit eed1ce7, which is an
> infrastructure piece to be able to enable and disable dynamically
> checksums on a cluster.  The main idea is to be able to bypass
> datallowconn which allows a background worker to connect to a database
> even if the database is set to refuse connections so as its checksums
> can be calculated and updated.
>
> At the end, the dynamic switch for checksums has been reverted as of
> a228cc13, and a set of rather-used APIs have been changed for what looks
> like no reason now:
> - BackgroundWorkerInitializeConnection
> - BackgroundWorkerInitializeConnectionByOid
> - InitPostgres
> So all background workers would not be able to compile because of that.
> Would we want to drop this unused interface or keep it?
>
> Even if this is not removed, bgworker.sgml needs to be updated with the
> new definition of BackgroundWorkerInitializeConnection and
> BackgroundWorkerInitializeConnectionByOid which are missing the third
> argument "uint32 flags", as well as the description for
> BGWORKER_BYPASS_ALLOWCONN.  I can personally see more reasons to revert
> that portion as well and consider it again for v12 or onwards if the
> on-line checksum switch is proposed again.
>
> Magnus, Daniel, what do you think?
>
> I think this feature is definitely worth keeping, regardless. It's useful elsewhere, in externally maintained
bgwriters.

+1, I think this is worth keeping in for 11.

cheers ./daniel

On Sun, Apr 22, 2018 at 03:56:32PM +0200, Daniel Gustafsson wrote:
> +1, I think this is worth keeping in for 11.

Okay, that's fine for me.  Thanks for updating the documentation.
Perhaps it would be nicer for plugin developers to add a comment in
bgworker.h as well about what BGWORKER_BYPASS_ALLOWCONN allows to
achieve?
--
Michael

On Mon, Apr 23, 2018 at 10:31:43AM +0200, Magnus Hagander wrote:
> Yeah, that makes sense. Will add as well.

Thanks for the addition!
--
Michael