Remove error for correct bootstrap of data node

[mkindahl]

If the database exists on the data node when executing add_data_node
it will generate an error in the data node log, which can cause
problems since there is an error indication in the log but there are no
failing operations.

This commit fixes this by first validating the database and only if it
does not exist, create the database.

Closes #2503

[codecov]

Codecov Report

Merging #2505 into master will increase coverage by 0.02%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #2505      +/-   ##
==========================================
+ Coverage   90.04%   90.06%   +0.02%     
==========================================
  Files         212      211       -1     
  Lines       34219    33729     -490     
==========================================
- Hits        30813    30379     -434     
+ Misses       3406     3350      -56     

Impacted Files	Coverage Δ
tsl/src/data_node.c	94.56% <92.85%> (-0.13%)	⬇️
src/plan_expand_hypertable.c	85.80% <0.00%> (-8.34%)	⬇️
src/loader/bgw_message_queue.c	85.52% <0.00%> (-2.22%)	⬇️
src/event_trigger.c	94.89% <0.00%> (-1.46%)	⬇️
src/hypertable_insert.c	95.18% <0.00%> (-1.21%)	⬇️
src/planner.c	93.31% <0.00%> (-0.50%)	⬇️
src/copy.c	87.44% <0.00%> (-0.43%)	⬇️
src/scanner.c	96.00% <0.00%> (-0.27%)	⬇️
tsl/src/data_node_dispatch.c	97.02% <0.00%> (-0.25%)	⬇️
src/chunk_dispatch_state.c	95.06% <0.00%> (-0.24%)	⬇️
... and 35 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65f3112...6bf5429. Read the comment docs.

[pmwkaa]

Change looks good.

I am slightly concerned about the overall if_not_exists logic here, since it tries to be smart and recreate non-existing objects step by step even if the foreign server exists on the AN, which suggests that DN potentially could be in non-sync state with AN. Basically it combines create and recovery (or healing) logic in one function, which might be not obvious and error prone in some sense. I guess dropping DN and adding it again would not work here as well, since drop does not drop DN's database.

[pmwkaa]

Why not 3? is it for future compatability?

[mkindahl]

No, this ensure that fields 0, 1, and 2 can be read, which are three elements.

[mkindahl]

Change looks good.

I am slightly concerned about the overall if_not_exists logic here, since it tries to be smart and recreate non-existing objects step by step even if the foreign server exists on the AN, which suggests that DN potentially could be in non-sync state with AN. Basically it combines create and recovery (or healing) logic in one function, which might be not obvious and error prone in some sense. I guess dropping DN and adding it again would not work here as well, since drop does not drop DN's database.

The if_not_exists behavior is only there because we have docker images that create the database and install the correct extension directly. If there are more stuff installed than then database and extension, the bootstrapping will fail so it is not possible add an old data node.

[pmwkaa]

Change looks good.
I am slightly concerned about the overall if_not_exists logic here, since it tries to be smart and recreate non-existing objects step by step even if the foreign server exists on the AN, which suggests that DN potentially could be in non-sync state with AN. Basically it combines create and recovery (or healing) logic in one function, which might be not obvious and error prone in some sense. I guess dropping DN and adding it again would not work here as well, since drop does not drop DN's database.

The if_not_exists behavior is only there because we have docker images that create the database and install the correct extension directly. If there are more stuff installed than then database and extension, the bootstrapping will fail so it is not possible add an old data node.

Wasn't using bootstrap = false in such case sufficient then?

[mkindahl]

Change looks good.
I am slightly concerned about the overall if_not_exists logic here, since it tries to be smart and recreate non-existing objects step by step even if the foreign server exists on the AN, which suggests that DN potentially could be in non-sync state with AN. Basically it combines create and recovery (or healing) logic in one function, which might be not obvious and error prone in some sense. I guess dropping DN and adding it again would not work here as well, since drop does not drop DN's database.

The if_not_exists behavior is only there because we have docker images that create the database and install the correct extension directly. If there are more stuff installed than then database and extension, the bootstrapping will fail so it is not possible add an old data node.

Wasn't using bootstrap = false in such case sufficient then?

No, because bootstrap creates a schema and adds data to the metadata table as well and if bootstrap is false, not all steps are done.

[niksajakovljevic]

LGTM