[FDC] Upgrate pglite and remove the ExtendQueryPatch
#9734
Conversation
fredzqm
changed the title
PGlitePatchExtendQueryPatch
Summary of ChangesHello @fredzqm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on updating the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades @electric-sql/pglite and removes the now-unnecessary logic from PGliteExtendedQueryPatch, which is a great cleanup. My main feedback is to fully remove the commented-out code from pgliteServer.ts to improve clarity. Additionally, since the PGliteExtendedQueryPatch class is no longer applying a patch, it should be renamed to better reflect its new role as a message logger. I also noticed a small typo ('Upgrate') in the pull request title.
| async *filterResponse(request: Uint8Array, response: Uint8Array) { | ||
| this.pgliteDebugLog.write( | ||
| `\n[-> ${getFrontendMessageCodeName(request[0])}] ` + decoder.write(request as any as Buffer), | ||
| ); | ||
|
|
||
| if (pipelineStartMessages.includes(message[0])) { | ||
| this.isExtendedQuery = true; | ||
| } | ||
| // const isExtendedQuery = | ||
| // request[0] === FrontendMessageCode.Parse || request[0] === FrontendMessageCode.Bind; | ||
| // let hasError = false; | ||
| // let hasReadyForQuery = false; | ||
|
|
||
| // 'Sync' indicates the end of an extended query | ||
| if (message[0] === FrontendMessageCode.Sync) { | ||
| this.isExtendedQuery = false; | ||
| this.eqpErrored = false; | ||
| } | ||
| // if (request[0] === FrontendMessageCode.Sync) { | ||
| // this.isExtendedQuery = false; | ||
| // this.extendedQueryErrored = false; | ||
| // } | ||
|
|
||
| // A PGlite response can contain multiple messages | ||
| // https://www.postgresql.org/docs/current/protocol-message-formats.html | ||
| for await (const bm of getMessages(response)) { | ||
| // After an ErrorMessage in extended query protocol, we should throw away messages until the next Sync | ||
| // (per https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-FLOW-EXT-QUERY:~:text=When%20an%20error,for%20each%20Sync.)) | ||
| if (this.eqpErrored) { | ||
| continue; | ||
| } | ||
| if (this.isExtendedQuery && bm[0] === BackendMessageCode.ErrorMessage) { | ||
| this.eqpErrored = true; | ||
| } | ||
| // if (this.eqpErrored) { | ||
| // this.pgliteDebugLog.write( | ||
| // `\n[<- ${getBackendMessageCodeName(bm[0])} (skipped)] ${decodedResp}`, | ||
| // ); | ||
| // continue; | ||
| // } | ||
| // if (bm[0] === BackendMessageCode.ErrorMessage) { | ||
| // hasError = true; | ||
| // } | ||
| // if (bm[0] === BackendMessageCode.ReadyForQuery) { | ||
| // hasReadyForQuery = true; | ||
| // } | ||
| // Filter out incorrect `ReadyForQuery` messages during the extended query protocol | ||
| if (this.isExtendedQuery && bm[0] === BackendMessageCode.ReadyForQuery) { | ||
| this.pgliteDebugLog.write("Filtered: " + decoder.write(bm as any as Buffer)); | ||
| continue; | ||
| } | ||
| this.pgliteDebugLog.write("Sent: " + decoder.write(bm as any as Buffer)); | ||
| // if (this.isExtendedQuery && bm[0] === BackendMessageCode.ReadyForQuery) { | ||
| // this.pgliteDebugLog.write( | ||
| // `\n[<- ${getBackendMessageCodeName(bm[0])} (skipped)] ${decodedResp}`, | ||
| // ); | ||
| // continue; | ||
| // } | ||
| this.pgliteDebugLog.write( | ||
| `\n[<- ${getBackendMessageCodeName(bm[0])}] ${decoder.write(bm as any as Buffer)}`, | ||
| ); | ||
| yield bm; | ||
| } | ||
| // if (isExtendedQuery && hasError && !hasReadyForQuery) { | ||
| // const bm = new Uint8Array([BackendMessageCode.ReadyForQuery, 0, 0, 0, 5, 73]); // 'I' = Idle | ||
| // yield bm; | ||
| // this.pgliteDebugLog.write( | ||
| // `\n[<- ${getBackendMessageCodeName(bm[0])} extra] ${decoder.write(bm as any as Buffer)}`, | ||
| // ); | ||
| // } | ||
| } |
There was a problem hiding this comment.
The filterResponse method contains a lot of commented-out code, likely from the old patch logic that is no longer needed after the pglite upgrade. This dead code should be removed to improve readability and maintainability.
Additionally, the class PGliteExtendedQueryPatch is no longer a patch and primarily functions as a logger. Consider renaming it to PGliteMessageLogger to more accurately reflect its purpose, since the PR title's goal of removing the patch hasn't been fully met by just commenting out the logic.
async *filterResponse(request: Uint8Array, response: Uint8Array) {
this.pgliteDebugLog.write(
`\n[-> ${getFrontendMessageCodeName(request[0])}] ` + decoder.write(request as any as Buffer),
);
// A PGlite response can contain multiple messages
// https://www.postgresql.org/docs/current/protocol-message-formats.html
for await (const bm of getMessages(response)) {
this.pgliteDebugLog.write(
`\n[<- ${getBackendMessageCodeName(bm[0])}] ${decoder.write(bm as any as Buffer)}`,
);
yield bm;
}
}1 participant
Removed the extended query patch because we switched FDC emulator to always use non-prepared SQL, so error can be properly surfaced back.
The upstream protocol bug still exists, but we are getting around it.