Check status of connection after PQconnectStartParams

[knizhnik]

Problem

See https://github.com/neondatabase/cloud/issues/14289

Summary of changes

Check connection status after calling PQconnectStartParams

Checklist before requesting a review

• I have performed a self-review of my code.
• If it is a core feature, I have added thorough tests.
• Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
• If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

• Do not forget to reformat commit message to not include the above checklist

[github-actions]

3000 tests run: 2885 passed, 0 failed, 115 skipped (full report)

---

Flaky tests (1)

Postgres 14

• test_location_conf_churn[3]: debug

Code coverage* (full report)

• functions: 32.7% (6914 of 21129 functions)
• lines: 50.1% (54189 of 108195 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
309c8b4 at 2024-07-01T12:17:18.817Z :recycle:}

[kelvich]

Thank you, good catch!

Looking at PQstatus code it seem that previous check is not needed and can be omitted (PQerrorMessage handles null pointer too).

ConnStatusType
PQstatus(const PGconn *conn)
{
	if (!conn)
		return CONNECTION_BAD;
	return conn->status;
}

Also that is a good example of how not to write commit messages: you are describing code change as such and that is visible from the PR itself in this case. What is not obvious is why this change is done (to fix cannot wait on socket event without a socket error)

[hlinnaka]

A test would be nice too

[knizhnik]

Also that is a good example of how not to write commit messages: you are describing code change as such and that is visible from the PR itself in this case. What is not obvious is why this change is done (to fix cannot wait on socket event without a socket error)

Sorry, writing good commit messages and comments was alway my problem.
But generally I thought that PR description should explain the source problem and how to reproduce it while commit message should explain how it is fixed. Should commit message be self-explained and contain both: description of the problem and its fix? Not obvious to me...

[knizhnik]

A test would be nice too

Yeh, it is non trivial. The errors seems to handle after OOM.
I do not know some convenient and reliable way to cause permanent OOM.
Certainly it is possible to execute queries creating huge strings and so consume all memory.
But once this query is canceled all this memory will be deallocated.

Also it OOM should happen in proper place. If PQconnectStartParams failed to allocate PGconn because of lack of memory, then it will return NULL which is already handled.

[kelvich]

Hm, you are still doing that previous not-null check, IIUC it is just code duplication now: #8210 (comment)

Yeh, it is non trivial. The errors seems to handle after OOM.

Hm, why? That WaitEventSet issue should happen if we pass broken connection down the code. In case of oom we would have null in connection and we would write an error message, so the test would have to e.g. try to connect to non-existent address? it still might be hard to reproduce it, but some analysis on how hard is it would be nice. Main benefit of test here would be to create a repro for that bug and check that code fixes it.

But generally I thought that PR description should explain the source problem and how to reproduce it while commit message should explain how it is fixed. Should commit message be self-explained and contain both: description of the problem and its fix? Not obvious to me...

Check last two days of commits in neon repo -- that would give a sense of overall style. It just easier if it is self-contained and later git blame shows reasoning right away.

[hlinnaka]

A test would be nice too

Yeh, it is non trivial. The errors seems to handle after OOM.

The libpq docs say:

To begin a nonblocking connection request, call PQconnectStart or PQconnectStartParams. If the result is null, then libpq has been unable to allocate a new PGconn structure. Otherwise, a valid PGconn pointer is returned (though not yet representing a valid connection to the database). Next call PQstatus(conn). If the result is CONNECTION_BAD, the connection attempt has already failed, typically because of invalid connection parameters.

Note the last sentence. So OOM is one way to hit this, but invalid connection parameters is much simpler to do in a test.

[knizhnik]

A test would be nice too

Yeh, it is non trivial. The errors seems to handle after OOM.

The libpq docs say:

To begin a nonblocking connection request, call PQconnectStart or PQconnectStartParams. If the result is null, then libpq has been unable to allocate a new PGconn structure. Otherwise, a valid PGconn pointer is returned (though not yet representing a valid connection to the database). Next call PQstatus(conn). If the result is CONNECTION_BAD, the connection attempt has already failed, typically because of invalid connection parameters.

Note the last sentence. So OOM is one way to hit this, but invalid connection parameters is much simpler to do in a test.

Sorry, I too harried to merge this PR.
Test is added I separate PR:
#8231