Closed deathwish closed 1 year ago
Can one of the admins verify this patch?
Test this, please
Test FAILed.
Hi @deathwish,
Thanks for the patch! It looks like there are some test failures. Could you please fix those? For example, see here: https://jenkins.juliogonzalez.es/job/tds_fdw-build-pr/15/DISTRO=centos7,PG_VER=11,label=docker/console
In tdsValidateForeignTableOptionSet(TdsFdwOptionSet *option_set)
, it looks to me like this condition might need to be tweaked:
if (option_set->use_remote_estimate < 0 || option_set->use_remote_estimate > 1)
Isn't the following condition correct for this context instead?:
if (option_set->use_remote_estimate < -1 || option_set->use_remote_estimate > 1)
if (option_set->use_remote_estimate < -1 || option_set->use_remote_estimate > 1)
The -1 value is supposed to be replaced by the option's default value in all cases, so I view the test failures as indicative. Ensuring defaults are set before option validation in all cases, as the commit I just pushed does, seems to fix the test failures and likely fixes other problems.
ETA: It's possible that tdsGetForeignServerTableOptions
should also validate the range of use_remote_estimate
. It would be reasonable to allow -1 in this case to explicitly select the default behavior.
Test this, please
Test PASSed.
This has been merged. Thanks again for the patch!
Great! Glad I could help, thanks for getting this in.
Previously, the code applying default settings would assume the option was unset, and set the default value of 1. We use a sentinel value instead, which allows us to detect this case separately.