Skip to content

Fix getColumnMeta() for GH-15287 Pdo\Pgsql::setAttribute(PDO::ATTR_PREFETCH, 0) - #23892

Closed
outtersg wants to merge 4 commits into
php:masterfrom
outtersg:gh-15287-columnmeta
Closed

outtersg wants to merge 4 commits into
php:masterfrom
outtersg:gh-15287-columnmeta

Conversation

@outtersg

Copy link
Copy Markdown
Contributor

As stated in the UPGRADING, using the passthrough ("single-row") mode of libpq (introduced in #15287) forbids passing a new query while the current one's results have not been entirely consumed.
… But I didn't notice that ext/pdo_pgsql internally used new queries to fetch metadata (example use case: a call to getColumnMeta() while fetch()ing row by row will interleave the getColumnMeta()-triggered internal query to the database, with the results fetching for the user-called query).

This PR:

  • prevents getColumnMeta() from interfering with an unbuffered user query
    by returning an empty value for every metadata that would emit an internal query, thus breaking libpq inner workings
  • caches the table name so that 30 calls to getColumnMeta() won't call the database 30 times

This is a second take to #16249; compared to it:

Of course I'd feel more comfortable if there was a way to run the full CI chain before merging: any hint from GH rgulars?

…lumnMeta()

- each call queried the DB to know the name associated with the table's OID:
  cache the result between two calls
- make pdo_pgsql_translate_oid_to_table higher-level,
  with the last parameter being the handle instead of the raw connection;
  thus the statement is cleaner, letting the handle do all memory handling on the table oid-to-name translation cache
  (which by the way is a driver feature more than a statement one)
@outtersg
outtersg force-pushed the gh-15287-columnmeta branch 2 times, most recently from b686801 to 14e94b7 Compare September 24, 2026 21:47
outtersg added a commit to outtersg/guili that referenced this pull request Sep 24, 2026
= Ajout des dernières corrections envoyées sur php/php-src#23892.

darcs-hash:0639e883821678f8f19a4670d86874658e2ba925
@devnexen

Copy link
Copy Markdown
Member

there are mix of fixes and cache improvements (thus should target different branches) but I ll have a better look this week end. Cheers !

…ment

we shall test if *the last statement* was lazy, not if the *new one* will
doing an (internal) query to fetch metadata from the server broke the currently-running (user) query
phpGH-23065 fixed running_stmt being only conditionally cleared in pgsql_stmt_finish(), by clearing it from the different callers of pgsql_stmt_finish();
put the clearing back into pgsql_stmt_finish(), albeit this time with the right condition to have all callers satisfied.
@outtersg

Copy link
Copy Markdown
Contributor Author

@devnexen wrote:

there are mix of fixes and cache improvements (thus should target different branches)
but I ll have a better look this week end. Cheers !

Thanks for the look!
Hoping that it will make reviewing easier this week-end, I reworded the commits, and resplit one of them.

Now we have:

  1. 207de5c clearly is a new feature; in fact it will benefit all use cases, not limited to ATTR_PREFETCH 0
    → new feature PR to master?
  2. d64c1f0 (extracted for the once big commit) is a small fix to test H->running_stmt->is_unbuffered instead of S->is_unbuffered.
    That's something I stumbled upon while making getColumnMeta() work, which it isn't related to, so it was better to give it its own commit.
    → new fix PR to 8.5? However I don't have a standalone test for it (it would theorically require running an unbuffered statement, not going all the way to the last result, then running a normal (ATTR_PREFETCH = 1) statement)
  3. ac31dff was initially called "fix getColumnMeta() for ATTR_PREFETCH = 0".
    However as I always sold ATTR_PREFETCH = 0 as coming with unspecified behaviour regarding interferences between multiple running statements (including internal ones), in other words "Good enough to quickly dump the results of a query to a CSV file, everything else may crash the driver",
    I regard making ATTR_PREFETCH = 0 compatible with every other feature of Pdo\Pgsql to be best effort.
    → so new feature PR to master?
  4. 9e25189 was initially a real fix (to a mis-nesting of an if into another one as seen in my original fix),
    but @KentarouTakeda fixed it in ext/pdo_pgsql: Fix several lazy fetch defects #23065, by fixing at the callers level;
    now my commit only factorizes back everything into the callee, so it is only a cleanup commit.
    → new cleanup PR to master? Or to 8.5? Or in the PR of 2.?

Tell me your preferred way of handling each part, so that I'll refactor the PR before you can comfortably review them.

@devnexen

Copy link
Copy Markdown
Member

To summarize, the cleanup part (pt 4) and the cache (pt 1) -> master, the rest -> 8.5

@outtersg

Copy link
Copy Markdown
Contributor Author

All 4 individual PRs created, as #23917, #23918, #23919, #23920.

@outtersg outtersg closed this Sep 25, 2026
outtersg added a commit to outtersg/php-src that referenced this pull request Sep 25, 2026
backport the parts of php#23920 that we relied upon (this PR being a split of php#23892)
kept it synchronized with the source, to ease later merging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants