Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ParameterDescription message to pg-protocol #2464

Merged
merged 1 commit into from Apr 6, 2021

Conversation

spacedentist
Copy link
Contributor

The PostgreSQL server replies with a series of messages, including a ParameterDescription message, when the description of a prepared statement is requested. This pull request implements parsing this message type according to the documentation of the PostgreSQL protocol (see https://www.postgresql.org/docs/12/protocol-message-formats.html).

@brianc
Copy link
Owner

brianc commented Feb 9, 2021

whoah can't believe I missed that message type..sorry about that. I'm assuming that prior to this change making it send a parameter description message was throwing an error?

I wonder if there's a way to write an integration test for this as well...in the pg library. I can take that on no problem...do you have a query or snippet of code that made the backend return that message type?

@spacedentist
Copy link
Contributor Author

whoah can't believe I missed that message type..sorry about that. I'm assuming that prior to this change making it send a parameter description message was throwing an error?

Yes, I was getting an exception.

I wonder if there's a way to write an integration test for this as well...in the pg library. I can take that on no problem...do you have a query or snippet of code that made the backend return that message type?

I can share with you what I have. What I am actually doing is to prepare an SQL statement (without ever executing it) and retrieve both parameter and row descriptions. The idea is that I want to validate SQL statements (at build time of my node.js app) and know what types they take as parameters and what they return.

Would be super cool if pg had an API for this, but I was able to hack it together without any changes in pg, I just needed the ParameterDescription to be supported.

So this is my code, if useful for you: first I create a client, but then hijack its connection and divert all incoming messages to my code:

  const client = new Client(config);
  await client.connect();

  const connection: Connection = (client as any).connection;

  // Check client is working
  const result = await client.query('SELECT 123 AS x;');
  console.assert(result.rows.length === 1);
  console.assert(result.rows[0]['x'] === 123);

  // Divert messages. First, add a `message` listener once, so that `message` events even get emitted
  connection.on('message', () => {});
  // Now, remove all existing listeners to this connection. The `client` won't hear from this connection anymore.
  connection.removeAllListeners();

  // Install my own listener for messages
  const receivedMessages: BackendMessage[] = [];
  const waitingCallbacks: Array<(msg: BackendMessage) => void> = [];

  connection.on('message', (msg: BackendMessage) => {
    if (waitingCallbacks.length) {
      const callback = waitingCallbacks.shift()!;
      callback(msg);
    } else {
      receivedMessages.push(msg);
    }
  });

  // Get the next message. If a message has been received already, immediately returns it.
  // If not, it returns a promise that will be resolved when the next message arrives.
  function getMessage(): Promise<BackendMessage> {
    if (receivedMessages.length) {
      return Promise.resolve(receivedMessages.shift()!);
    } else {
      return new Promise<BackendMessage>((resolve) => {
        waitingCallbacks.push(resolve);
      });
    }
  }

With that in place, I have this function that takes any SQL query string and returns parameters and columns for it, without actually executing the query. The query string may contain placeholders ($1 etc.), but you don't need to provide any values for those.

async function getQueryInformation(query: string) {
    const serial = `s${count++}`;

    connection.parse({ name: serial, text: query, types: [] }, true);
    const prom1 = getMessage();
    connection.describe({ type: 'S', name: serial }, true);
    const prom2 = getMessage();
    connection.flush();
    const prom3 = getMessage();

    const parseCompleteMsg = await prom1;
    if (!isParseCompleteMessage(parseCompleteMsg)) {
      throw new Error('Expected ParseComplete Message');
    }

    const parameterDescriptionMsg = await prom2;
    if (!isParameterDescriptionMessage(parameterDescriptionMsg)) {
      throw new Error('Expected ParameterDescription Message');
    }

    const rowDescriptionMsg = await prom3;
    if (!isRowDescriptionMessage(rowDescriptionMsg)) {
      throw new Error('Expected RowDescription Message');
    }

    return {
      parameters: parameterDescriptionMsg.dataTypeIDs,
      columns: rowDescriptionMsg.fields.map(
        ({ name, dataTypeID, tableID, columnID }) => ({
          name,
          dataTypeID,
          tableID,
          columnID,
        }),
      ),
    };
  }

Like I was saying, it's a hack. But it does work, and it's really valuable for me. It allows me to check SQL query literals for soundness, and also get type information. This can run as a build step, because it doesn't actually do anything to the database, as no queries are ever executed.
If you want to take this as inspiration for a new pg feature (I mean, just Client.getQueryInformation(query:string) would be great to have!), I would be super happy for that.
Getting this pull request merged (and released) would already make my life easier, because then my hacks works without depending on my local build of pg.

PS: For completeness:

import { Client, Connection, ClientConfig } from 'pg';
import {
  BackendMessage,
  MessageName,
  RowDescriptionMessage,
  ParameterDescriptionMessage,
} from 'node_modules/pg-protocol/src/messages';

function isParseCompleteMessage(msg: BackendMessage) {
  return msg.name === MessageName.parseComplete;
}
function isParameterDescriptionMessage(
  msg: BackendMessage,
): msg is ParameterDescriptionMessage {
  return msg.name === MessageName.parameterDescription;
}
function isRowDescriptionMessage(
  msg: BackendMessage,
): msg is RowDescriptionMessage {
  return msg.name === MessageName.rowDescription;
}

let count = 0;

(All the code snippets in this comment are MIT licensed - feel free to use whatever is useful!)

@brianc
Copy link
Owner

brianc commented Mar 18, 2021

@spacedentist sorry for sleeping on this - a bunch of stuff happened all at once including snowpocolyps knocking me off the power grid in austin for a few days...then I was just buried in life duties for a while. Do you want to take a crack at resolving this merge conflict or do you want me to? Afterwards I'll merge & push out a new minor version w/ this long overdue change.

@danieldunderfelt
Copy link

I tried applying the changes from this PR with patch-package, but I'm getting AssertionError [ERR_ASSERTION]: unknown message code: 74 when trying to run the testing code (as provided by @spacedentist). Are there other steps that I need to do I am not aware of? I added all changes except for the stuff in packages/pg-protocol/src/inbound-parser.test.ts.

The error comes from pg-protocol/src/parser.ts:198 when running with ts-node and pg-protocol/dist/parser.js:138 when running the built js files with node, so I'm sure it is picking up the changes and I don't think I need to run separate build steps for node-postgres.

The original issue I had before going down this rabbit hole is that I get bind message has x parameter formats but 0 parameters when trying to insert stuff. This is NOT the more common "bind message supplies..." error, and I need this PR to look into what's happening with my query. My parameter list is not empty, as the error message would imply.

@spacedentist
Copy link
Contributor Author

Hi @brianc!

Sorry for being so unresponsive. The GitHub notification must have got drowned in notification noise, just checked back on the page and saw your message.

The merge conflict was easy to fix, so I've updated my commit. Would be great if you could merge & release.

Hope Texas has fully recovered from the snow. We have just had the first two days of spring-like weather here in London.

@danieldunderfelt - I am wondering if your problems were related to the merge conflict. The "unknown message code 74" is exactly what this PR tries to resolve. I had to make tiny changes in two places to make this apply to current master (and work!), so maybe that's what broke it for you.

@brianc
Copy link
Owner

brianc commented Apr 6, 2021

Hey great! Yeah I understand the thing about github notification emails. I'll merge this & push out a semver minor in an hour or two. Thanks!

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.

None yet

5 participants