Skip to content

async connect#141

Merged
turnstep merged 2 commits intobucardo:masterfrom
rweikusat:async-connect-for-merge
Dec 10, 2025
Merged

async connect#141
turnstep merged 2 commits intobucardo:masterfrom
rweikusat:async-connect-for-merge

Conversation

@rweikusat
Copy link
Contributor

Support for asynchronous connect via PQconnectStart/ PQconnectPoll by passing a special attrbitute (pg_async_connect) to DBI::connect.

This is identical to the earlier async connect pull request but has all changes as single commit as my internal development history is probably not that interesting (or necessarily easy to follow).

@esabol
Copy link
Collaborator

esabol commented May 21, 2025

This is identical to the earlier async connect pull request but has all changes as single commit as my internal development history is probably not that interesting (or necessarily easy to follow).

Traditionally, the proper way to fix that is to rebase your branch and squash the commits. Do a web search to learn how to do that or watch this video: https://youtu.be/8j0H6urZ-bU?si=UinF0MCCK9QBRW0y. It's much tidier to do that than to close a PR and open a new PR. In practice you don't need to do that, however. GitHub added the capability to squash commits when merging to the web interface years ago, so project maintainers can take care of that. Some people still like to squash their commits themselves, of course. It looks cleaner, but it's usually unnecessary. Some projects still require contributors to squash their commits before merging (check the project's contributing guidelines if they have any), but that's kind of old fashioned.

I would recommend renaming this pull request to remove the "as a single commit" part from the title of the PR. It's kind of confusing when dealing with transactional databases, and it's not meaningful in terms of the merge history, when/if it gets merged.

@rweikusat
Copy link
Contributor Author

Thanks for the feedback. But this is just supposed to be something that's easier to review as a whole, meaning, I'm planning to keep the full history for myself (which has already been rebased quite a bit to squash nuisance commits like fixes for odd typos).

I'll change the name.

@rweikusat rweikusat changed the title async connect changes as single commit async connect May 21, 2025
@turnstep turnstep merged commit 48825dd into bucardo:master Dec 10, 2025
@esabol
Copy link
Collaborator

esabol commented Dec 11, 2025

@turnstep @rweikusat : Merging this PR caused the CI workflow to fail tests in t/99_lint.t and t/99_perlcritic.t and t/99_spellcheck.t. Please fix. I advise running the CI workflow on each PR prior to merging.

@esabol
Copy link
Collaborator

esabol commented Dec 11, 2025

The bigger deal is the test failure in t/03dbmethod.t.

https://github.com/bucardo/dbdpg/actions/runs/20106092843/job/57690271198#step:9:152

@rweikusat
Copy link
Contributor Author

I can't promise a resolution today, but I'm looking into this.

@turnstep
Copy link
Contributor

Hmm...CI should probably not be running the lint/critic/spellcheck tests, those are meant for authors only. Need to look into that.

@esabol
Copy link
Collaborator

esabol commented Dec 11, 2025

Hmm...CI should probably not be running the lint/critic/spellcheck tests, those are meant for authors only. Need to look into that.

I don't see any reason not to run those tests. IMHO, I would keep that as-is.

The annoying thing is that the CI workflow doesn't run automatically whenever a PR is opened. It's requiring manual approval. I think that can be changed, but I'm not sure.

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#configuring-required-approval-for-workflows-from-public-forks

@rweikusat
Copy link
Contributor Author

The pull request I just created (#152) should fix the pg_db_cancel issue. It doesn't address this 'failed to create dbh for sync connect test' issue. This works for me when running the test suite locallly. I realize this doesn't help much, ;-), but since it's 7:45pm here, I'll look into that tomorrow.

@rweikusat
Copy link
Contributor Author

Update on this: This is a test case I added which should do a synchronous connect when pg_async_connect => 0 was passed to the connect method. This works on my computer at home aka "the development machine." I'll see if I can reproduce this somehow via GitHub actions without abusing the original repository for that. So far, this fails because of a different "mystery error" and since I'm completely new to GitHub workflows, I have no idea how to fix this yet (or why it's happening).

OTOH, since I don't really have anything to do at the moment (except job applications which seem entirely pointless) and was looking for a new technology project, anyway, I'll continue working on this although probably not before Monday.

@rweikusat rweikusat deleted the async-connect-for-merge branch December 19, 2025 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants