Add a new option for gpexpand to bypass checking the icproxy address

[longforrich]

Because the 'icproxy' is a configurable function. So, if we have not config it, give us a chance to bypass it.

Here are some reminders before you submit the pull request

• [ ] Add tests for the change
• [x] Document changes
• [ ] Communicate in the mailing list if needed
• [x] Pass make installcheck
• [ ] Review a PR in return to support the community

[ashwinstar]

@kainwen Please can team look into it. I don't think it should be a option from user - the check should only apply if icproxy is configured.

[kainwen]

@kainwen Please can team look into it. I don't think it should be a option from user - the check should only apply if icproxy is configured.

The check is subtle. For now, even users don't use icproxy, it still start the background worker because historical reasons that interconnect type can be PGOPTIONs for sessions.

So two things here:

1. we should do the correct check that: if icproxy addrs is set or set, this means if customer really use or not icproxy
2. we need to see for users not use icproxy, do we really want to spawn the background worker?

[interma]

I wrote about the related logic previously. In my option, it's no need to add a new option for it. Checking ICProxy address has no side effects, it only does the special check logic when gp_interconnect_type==proxy and gp_interconnect_proxy_addresses have been set: https://github.com/greenplum-db/gpdb/blob/b5f82e709fccc6110ec5539f6e6899e08aa71080/gpMgmt/bin/gpexpand#L2002 Usually, it does nothing when the user doesn't enable Proxy mode though the IC process starts.

And the two points which Zhenghua mentioned:

we should do the correct check that: if icproxy addrs is set or set, this means if customer really use or not icproxy

Yes, the current implementation is correct.

we need to see for users not use icproxy, do we really want to spawn the background worker?

I think it's a further improvement. As far as I know, the gp_interconnect_type and gp_interconnect_proxy_addresses can be set dynamiclly and no need to let the cluster restart, so we always start IC process.

[interma]

@longforrich please take a look at my previous comment. Based on it, I think this PR is not necessary. Or do you find any special scenario or bug related to it? Thanks.

[longforrich]

the gp_interconnect_type and gp_interconnect_proxy_addresses can be set dynamiclly and no need to let the cluster restart, so we always start IC process.

this GUC is protected by the macro 'ENABLE_IC_PROXY' in src/backend/utils/misc/guc_gp.c. If the IC_PROXY is not configured that let's the gpexpand falls with 'unrecognized configuration parameter' in ‘show gp_interconnect_proxy_addresses ’ at the function validate_icproxy_addr.

so.. don't want to add a new option, maybe we can initialize both GUCs with invalid and read-only values, when not configured IC_PROXY ?

[interma]

If the IC_PROXY is not configured that let's the gpexpand falls with 'unrecognized configuration parameter' in ‘show gp_interconnect_proxy_addresses ’ at the function validate_icproxy_addr.

You are right, thanks for your explanation.

[interma]

And this change needs to backport to 6x, I will do it later.

[interma]

@longforrich Could you help to do another minor improvement in this PR?

another fix method is checking the Guc if exists in validate_icproxy_addr()

in validate_icproxy_addr():

        sql = "show gp_interconnect_proxy_addresses;"
        conn = dbconn.connect(_gp_expand.dburl, encoding='UTF8')
        icproxy_addresses_string = dbconn.queryRow(conn, sql)[0].strip()

If find error here, treat it as icproxy_addresses_string="", so we don't bother user to notice it (and rerun with --without-icproxy-check). Thanks!

[interma]

Merged this PR. And I will do the improvement (see my previous comment) later and backport necessary changes to 6x.

[longforrich]

sorry.... no more free time in the past to deal with this MR, but now I have time to do this. Do I need to enhance it?

[interma]

Do I need to enhance it?

Thanks, no need for now, your PR has been merged (And I will do the small enhancement at a proper time (just two or three lines), don't bother you to do it)

Welcome for the further contribution to GP.